incubator-stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Black <abl...@roguewave.com>
Subject Re: [PATCH] stdcxx target warning counting
Date Wed, 11 Oct 2006 22:21:37 GMT
Revised patch, attached.  I believe the same change log can be used.

Martin Sebor wrote:
> Andrew Black wrote:
>> Greetings all.
>>
[...]
>> +    "    icc  Intel icc\n"
> 
> Does this handle the Windows version of the compiler or just
> the Linux one? If both, we should change the text to Intel C++
> to make it clear (or add an alias for icl).

This handles just the Linux version of the compiler.  I added 'for 
Linux' to the end of the name in the revised patch in an effort to 
provide clarity

[...]
> 
> You could also add aliases if you like (e.g., for IBM VisualAge,
> we could have "vacpp IBM VisualAge C++" in addition to XLC++).

I had been trying to stick with 3-4 letter alias names for a measure of 
consistency, but clarity in naming convention is more important.  I did 
add the vacpp entry in addition to the xlc entry, but I didn't note that 
they were equivalent, outside of a comment in the translation table.

[...]
> We could probably just compare the names without regard to case,
> don't you think? Or did you have a specific reason for recognizing
> only these three forms?

One version of the patch did do a case insensitive comparison, moving 
the rw_strcasecmp (and rw_charcasecmp) functions from exec.cpp to 
util.cpp.  The reason I chose to do things this way was so that 
different warning parsing modes could be specified for the compile and 
link stages.  An example where this could be useful would be when 
building with GCC on Solaris, but linking with Sun's linker.

If we chose to restrict things to a single tool chain (ie: GCC or 
sunpro, but not mixing them), I would agree that it would make sense to 
match in a case-insensitive manner.  Looking at the config files seems 
to indicate to me that we are using a single tool chain normally, but I 
wonder if this can be assumed to always be true.  My suspicion is that 
we shouldn't.

> 
> [...]
>> Index: util/output.cpp
>> ===================================================================
>> --- util/output.cpp    (revision 454401)
>> +++ util/output.cpp    (working copy)
>> @@ -109,7 +109,13 @@
>>                      return;
>>                  }
>>              }
>> -            /* else if (1 < r_lvl) warning*/
>> +            else if (1 < r_lvl) {
>> +                status->t_warn += r_active;
>> +                if (status->t_warn < r_active) {
>> +                    status->status = ST_OVERFLOW;
> 
> Is this arithmetic overflow? (How long would it take for the counter
> to reach 4 billion and how big would the input file have to be in
> order for it to happen? I suspect we can drop this check and keep
> it simple.)
> 

This is a math overflow check.  If we assume ~5 assertions take 1 k of 
disk space in a result log (each assertion ~200 characters (bytes) 
long), the resulting file would be ~800,000,000 kb, or ~800 Gb.  A file 
this long is theoretically within the capabilities of a modern OS, but 
you are likely to run out of disk space prior to writing the entire log.

This check was added for consistency with the assertion counting code, 
and it would be outside the scope of this patch to remove that check 
(and the ST_OVERFLOW item).

> [...]
> 
>> Index: util/display.cpp
>> ===================================================================
>> --- util/display.cpp    (revision 454401)
>> +++ util/display.cpp    (working copy)
>> @@ -40,8 +40,8 @@
>>  */
>>  static void print_header_plain ()
>>  {
>> -    puts ("NAME                      STATUS ASSERTS FAILED PERCNT    
>> USER     "
>> -          "SYS    WALL");
>> +    puts ("NAME                      STATUS WARN ASSERTS FAILED 
>> PERCNT    "
>> +          "USER     SYS    WALL");
> 
> I would suggest to rename WALL to REAL as that's what the times
> command reports (and it's probably more a well known term than
> WALL alone).
> 

I would agree with renaming the column header, but I feel doing so would 
also be outside the scope of this patch.  If it were to be mixed in with 
another commit, the patch that cleans up the timing methods would be a 
logical candidate, but would introduce a conflict between this patch and 
that one.

[...]
> [...]
>> @@ -212,10 +282,41 @@
>>  {
>>      struct stat file_info;
>>      int exists = 1;
>> +    size_t path_len;
>> +    char* tmp_name;
>>  
>>      assert (0 != target);
>>      assert (0 != status);
>>  
>> +    path_len = strlen (target);
>> +
>> +#if !defined (_WIN32) && !defined (_WIN64)
> 
> As I mentioned before, these conditionals can be simplified to
> just #ifndef _WIN32 since _WIN32 is guaranteed to be defined in
> 64-bit mode.

This is part relocation artifact, part (misguided) consistency with 
other parts of code that use the same construct.  I'm still not certain 
about the guarantee, but have been unsuccessful in compiling on 64 bit 
windows.

> 
>> +    /* Otherwise, check for the .o file on non-windows systems */
>> +    tmp_name = (char*)RW_MALLOC (path_len + 3);
> 
> Is there a call to free() corresponding to this malloc()? (I
> don't see it in the patch but it might be in the rest of the
> code).

There was a free in one part of the code path, but the other (more 
common) paths lacked the call to free.  (This was a result of moving the 
code block from within a condition to an earlier, unconditional, part of 
the code flow.)

> 
> Martin

--Andrew Black

Mime
View raw message