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@lists.launchpad.net > > Unsubscribe : https://launchpad.net/~kicad-developers > > More help : https://help.launchpad.net/ListHelp > > > > > _______________________________________________ > Mailing list: https://launchpad.net/~kicad-developers > Post to : kicad-developers@lists.launchpad.net > Unsubscribe : https://launchpad.net/~kicad-developers > More help : https://help.launchpad.net/ListHelp _______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
