kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxx>]

On 6/26/2015 8:02 PM, Chris Pavlina wrote:
> The warning is definitely valid, for the reason I explained. You're 
> comparing a negative value to a value that is allowed to (and explicitly 
> declared to, in some compiler versions according to a preprocessor 
> directive) be unsigned. Compilers can, and some will, assume that the 
> comparison is false because an unsigned value can never be negative. 
> This isn't a bogus warning.

While agree with you on a technical level, reality may be somewhat
different.  Do you have any evidence that the existing code is producing
buggy behavioral results within the layer manipulation code that uses
this enumeration?  If so, than this patch is a no brainer and I will
commit it.  If not, it fixes nothing other than a compiler warning.

> 
> In any case, I've no problem with only posting true bugfix patches from 
> now on.

Not from now on.  Only until after the stable release.  Once the stable
release is done, we can resume with new features and compiler warning fixes.

> 
> On Fri, Jun 26, 2015 at 07:54:26PM -0400, Wayne Stambaugh wrote:
>> I committed patches 1 & 2 for the time being.  I'm not completely sold
>> enum part of patch 3.  Using a word like tautological always makes me
>> suspicious. :).  Please don't assume a compiler warning is bad code and
>> that the person who wrote the code didn't understand what they were
>> doing.  Too many developers put way too much stock in compiler warnings.
>>  Sometimes they are valid but more often than not they are benign or
>> worse noise that distracts developers from real issues.  From now until
>> the stable release, please only post patches that fix actual bugs.
>>
>> Thanks,
>>
>> Wayne
>>
>> On 6/26/2015 1:58 PM, Chris Pavlina wrote:
>>> Hi,
>>>
>>> Attached are three patches to clean up some compiler warnings from clang.
>>>
>>> 1: replace abs() with std::abs() in polygon/math_for_graphics.cpp
>>>
>>> This is the correct usage. The math is being done from double to double, 
>>> but abs() is an integer function. std::abs() will return the correct 
>>> types.
>>>
>>> 2: delete a couple unused variables
>>>
>>> 3: correct a couple tautological comparisons
>>>
>>> By defining UNDEFINED_LAYER and UNSELECTED_LAYER as follows:
>>>
>>> #define UNDEFINED_LAYER     LAYER_ID(-1)
>>> #define UNSELECTED_LAYER    LAYER_ID(-2)
>>>
>>> "enum LAYER_ID" is allowed to be an unsigned type (in fact, it is 
>>> explicitly declared to be one). A compiler is totally free to assume 
>>> that any comparison between this unsigned enum and a negative value is 
>>> false by definition and not actually perform the comparison. Currently 
>>> we aren't having this problem, but because it's allowed, it could become 
>>> a problem in the future with different or newer compilers and different 
>>> optimization settings. I removed the explicit declaration of the enum as 
>>> unsigned and defined UNDEFINED_LAYER and UNSELECTED_LAYER _in_ the enum.
>>>
>>> In polygon/poly2tri/sweep/sweep.cc, the address of a reference is tested 
>>> against NULL. This is a similar tautological comparison because C++ 
>>> explicitly disallows references to NULL. I changed this to a pointer 
>>> type (which had the added benefit of making it more consistent with the 
>>> other functions in the file).
>>>
>>> --
>>> Chris
>>>
>>>
>>>
>>> _______________________________________________
>>> Mailing list: https://launchpad.net/~kicad-developers
>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>> Unsubscribe : https://launchpad.net/~kicad-developers
>>> More help   : https://help.launchpad.net/ListHelp
>>>
>>
>>
>> _______________________________________________
>> Mailing list: https://launchpad.net/~kicad-developers
>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>> Unsubscribe : https://launchpad.net/~kicad-developers
>> More help   : https://help.launchpad.net/ListHelp
> 
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
>