kicad-developers team mailing list archive

[Chris Pavlina <pavlina.chris@xxxxxxxxx>]

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.

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

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