On Fri, Jun 26, 2015 at 08:12:21PM -0400, Wayne Stambaugh wrote: > 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.
Nope - I specifically said, it doesn't produce any known bugs _now_, but it will with different compilers or compiler settings and so is a potential trap for the future. I will definitely argue that this patch is a Good Thing and cleans up the code - the way it's written now is quite bizarre - but if you want to do only /true/ bugfixes and not cleanup until stable, that's fine by me, I'll sit on it. > > > > > 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. Yeah, that's what I meant :) > > > > > 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 > > > > > _______________________________________________ > 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
