incubator-stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Martin Sebor <se...@roguewave.com>
Subject Re: [PATCH] stdcxx target warning counting
Date Thu, 12 Oct 2006 19:49:30 GMT
Andrew Black wrote:
> Revised patch, attached.  I believe the same change log can be used.
> 
[...]
> 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.

That's a good point. But I'm afraid I'm not sure I see how
capitalization can help with which linker is used.

> 
> 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.

Changing the linker is typically only possible with gcc (which
doesn't mean that other compilers couldn't at least in theory
use different linkers). AFAIK, gcc uses the linker it is
configured for and it doesn't make it possible to change it
once it's been installed. That said, it's certainly possible
to have two installations of gcc, say on Solaris, with one
configured to use the native linker and another to use the GNU
linker. To correctly handle these two cases without user input
(and without additional autodetection machinery) I suspect it
might be easiest to avoid making assumptions about the format
of linker warnings when using gcc (or any compiler that allows
non-default linkers).

[...]
> 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).

Maybe. It doesn't mean that we need to introduce unnecessary logic
into new code just for consistency with existing code. I don't
suppose you would introduce a bug into new code just because we
had the same bug in a similar piece of existing code, would you? :)
Incidentally, if you insist on handling this exceedingly unlikely
case, it would be better to indicate the overflow in the column
that overflowed (since there are more than one) and not in the
status column.

[...]
>> 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.

It's an excellent policy to follow in general. In cases like this
one (a trivial change) the policy should be balanced against the
overhead involved in submitting a separate patch. Someone needs
to review the patch and someone needs to commit it. It would save
someone time and energy if you would bend the rules just a little
and made the change in the same patch :)

[...]
>> 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,

Read the documentation to convince yourself :)
http://msdn2.microsoft.com/en-us/library/b0084kay.aspx

If you're planning to go back and simplify all these conditionals
in a separate patch that's fine, but as I said above, there's no
need to perpetuate the more complicated logic in new code.

Martin


Mime
View raw message