On Wed, Oct 17, 2012 at 2:08 PM, Richard Trieu <[email protected]> wrote:
> 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. > No, sorry. I'm neither suggesting it, or suggesting not doing it, I simply didn't check which the patch did and wanted to cover all my bases. =] Feel free to ignore any cases in my comment which simply don't apply. =] > >> 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
