kicad-developers team mailing list archive

[Chris Pavlina <pavlina.chris@xxxxxxxxx>]

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@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
> > 
> 
> 
> _______________________________________________
> 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