On Tue, Oct 16, 2012 at 9:35 PM, Chandler Carruth <[email protected]>wrote:
> On Tue, Oct 16, 2012 at 3:34 PM, Richard Trieu <[email protected]> wrote: > >> Changed the fix-it to suggest parentheses around the comparison instead >> of switching the comparison operator. > > > I don't think that's the correct fix in all cases. I'd rather detect when > either of the candidate operands are floating point, and condition which > fixit we display on that. > > Additionally, we should test whether either operand is a class type and > lacking a definition of the inverted condition, or whether we are inside of > the definition of the inverted condition. > Are you suggesting this warning be extended to class types? Currently, the warning only checks for the built-in logical not operator. Overloaded operators are treated as function calls and won't trigger this warning. > > The reason I prefer this is that I'd like to have a narrow list of > exclusions to the less verbose fixit hint. When these are integers, it > seems like our fixit is going to be less useful if it will only insert > parentheses. > > -Chandler > > >> logical-not.cc:2:7: warning: logical not is only applied to the left hand >> side >> of this comparison [-Wlogical-not-compare] >> if (!5 < 4) >> ^ ~ >> logical-not.cc:2:7: note: add parentheses after the '!' to evaluate the >> condition first >> if (!5 < 4) >> ^ >> ( ) >> logical-not.cc:2:7: note: add parentheses around left hand side >> expression to >> silence >> if (!5 < 4) >> ^ >> ( ) >> >> On Mon, Oct 15, 2012 at 12:37 PM, Sean Silva <[email protected]> wrote: >> >>> > First, we have a fixit for (!expr1 < expr2). I'm recommending >>> suggesting that maybe the user intended to write !(expr1 < expr2) instead, >>> which is always at least as correct as the other proposed fixit, (expr1 >= >>> expr2), and sometimes significantly better (if either expr is >>> floating-point). >>> >>> I agree, the intention of the programmer was clearly !(x<5). >>> >>> > More to the point, you're conflating two separate issues. >>> >>> I must have miscommunicated, as my point was unrelated to either of >>> those things. Really what I was trying to get across is that even the >>> suggestions of !(x<5) could lead to a "subtle bug", Re your comment: >>> "We don't want to find bugs in users' code only to replace them with >>> even more subtle bugs". Even if the conversion is not UB (thanks for >>> correcting me there), it is still likely to introduce a subtle bug. >>> >>> I think that the criterion for the suggestion should be "what the >>> programmer intended", so that !(x<5) is clearly the preferable >>> suggestion. IMO suggesting x>=5 goes a bit beyond the realm of >>> "programmer intent" and into "trying to simplify your code with a >>> suggestion as to how you should phrase a comparison". >>> >>> -- Sean Silva >>> >>> On Mon, Oct 15, 2012 at 2:07 PM, Stephen Canon <[email protected]> wrote: >>> > >>> > On Oct 15, 2012, at 1:37 PM, Sean Silva <[email protected]> wrote: >>> > >>> >>> The compiler shouldn't be in the business of suggesting semantically >>> incorrect transformations to the user, especially when there's a perfectly >>> good alternative available. >>> >>> >>> >>> We don't want to find bugs in users' code only to replace them with >>> even more subtle bugs. For example, conver >>> >> >>> >> While I agree with you, there is a certain balance to be had. Even >>> >> converting it to !(x < 5) could cause issues if the conversion between >>> >> integer and float causes undefined behavior by not being >>> >> representable. I forget which direction the conversion for (f < 5) >>> >> would go (int->float or float->int), but either operand could be >>> >> outside the domain of the other (replacing 5 by INT_MAX, for example, >>> >> or having f be 1e30). >>> > >>> > int -> float, and conversions from int to float are always defined >>> (assuming IEEE-754 floating point). The conversion may be inexact, and may >>> even overflow on platforms with 128-bit integers, but it is always defined. >>> > >>> > More to the point, you're conflating two separate issues. >>> > >>> > First, we have a fixit for (!expr1 < expr2). I'm recommending >>> suggesting that maybe the user intended to write !(expr1 < expr2) instead, >>> which is always at least as correct as the other proposed fixit, (expr1 >= >>> expr2), and sometimes significantly better (if either expr is >>> floating-point). >>> > >>> > Possibly warning the user of inexact conversions of integer constants >>> to floating-point is an entirely separate issue (worth investigating; it >>> would be neat if the compiler warned on (float)x < INT_MAX by pointing out >>> that the comparison is actually being performed against 0x80000000.0 >>> instead of 0x7ffffff.0, which isn't representable in single-precision, but >>> that's an entirely separate can of worms). >>> > >>> > - Steve >>> >>> _______________________________________________ >>> cfe-commits mailing list >>> [email protected] >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>> >> >> >> _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
