On Mon, Nov 12, 2012 at 6:35 PM, Eli Friedman <[email protected]>wrote:
> On Mon, Nov 12, 2012 at 5:31 PM, Richard Trieu <[email protected]> wrote: > > I uploaded a new patch. > > > > On Mon, Nov 12, 2012 at 11:22 AM, Eli Friedman <[email protected]> > > wrote: > >> > >> Comments below. > >> > >> On Mon, Nov 12, 2012 at 9:46 AM, Richard Trieu <[email protected]> > wrote: > >> > Make -Wtautological-constant-out-of-range-compare checking take into > >> > account types and conversion between types. The old version merely > checked > >> > the bit widths, which allowed failed to catch a few cases, while > warning on > >> > other safe comparisons. > >> > > >> > http://llvm-reviews.chandlerc.com/D113 > >> > > >> > Files: > >> > test/Analysis/additive-folding.cpp > >> > test/SemaCXX/compare.cpp > >> > test/SemaCXX/warn-enum-compare.cpp > >> > lib/Sema/SemaChecking.cpp > >> > >> Index: test/SemaCXX/warn-enum-compare.cpp > >> =================================================================== > >> --- test/SemaCXX/warn-enum-compare.cpp > >> +++ test/SemaCXX/warn-enum-compare.cpp > >> @@ -39,8 +39,8 @@ > >> while (b == c); > >> while (B1 == name1::B2); > >> while (B2 == name2::B1); > >> - while (x == AnonAA); // expected-warning {{comparison of constant > >> 42 with expression of type 'Foo' is always false}} > >> - while (AnonBB == y); // expected-warning {{comparison of constant > >> 45 with expression of type 'Bar' is always false}} > >> + while (x == AnonAA); > >> + while (AnonBB == y); > >> while (AnonAA == AnonAB); > >> while (AnonAB == AnonBA); > >> while (AnonBB == AnonAA); > >> > >> Why are you changing this warning? > > > > See discussion below at IntRange. > >> > >> > >> while (AnonBB == AnonAA); > >> Index: lib/Sema/SemaChecking.cpp > >> =================================================================== > >> --- lib/Sema/SemaChecking.cpp > >> +++ lib/Sema/SemaChecking.cpp > >> @@ -4328,38 +4328,91 @@ > >> Expr *Constant, Expr *Other, > >> llvm::APSInt Value, > >> bool RhsConstant) { > >> + // 0 values are handled later by CheckTrivialUnsignedComparison(). > >> + if (Value == 0) > >> + return; > >> + > >> BinaryOperatorKind op = E->getOpcode(); > >> QualType OtherT = Other->getType(); > >> QualType ConstantT = Constant->getType(); > >> + QualType CommonT = E->getLHS()->getType(); > >> if (S.Context.hasSameUnqualifiedType(OtherT, ConstantT)) > >> return; > >> assert((OtherT->isIntegerType() && ConstantT->isIntegerType()) > >> && "comparison with non-integer type"); > >> // FIXME. handle cases for signedness to catch (signed char)N == 200 > >> > >> Does this patch fix this FIXME? > > > > Yes. FIXME removed, added test case. > >> > >> > >> - IntRange OtherRange = IntRange::forValueOfType(S.Context, OtherT); > >> - IntRange LitRange = GetValueRange(S.Context, Value, > >> Value.getBitWidth()); > >> - if (OtherRange.Width >= LitRange.Width) > >> + > >> + bool ConstantSigned = ConstantT->isSignedIntegerType(); > >> + bool OtherSigned = OtherT->isSignedIntegerType(); > >> + bool CommonSigned = CommonT->isSignedIntegerType(); > >> > >> Why are you getting rid of the IntRange usage? Granted, we didn't > >> really have any good testcases, but it does add power to the analysis > >> in some cases. > > > > For my use, IntRange didn't add any value. Everything I needed, I could > > just get from the APSInt. The only difference was what width I used for > > Other. IntRange does truncate the enum width so that different values > will > > be considered out of range when comparing against enums. I factored out > the > > width into a variable. The current code will remove the existing > warnings > > in warn-enum-compare.cpp. The code in the comments would have preserved > > them. > > It does add value in cases other than enums; it can recognize > bitfields and knows some tricks with binary "&", logical operators in > C, etc. > > IntRange doesn't appear to take those special cases into consideration. From looking at the code, it appears to special case enum, otherwise it falls back to ASTContext::getIntWdith(). > Again, if you want to change our enum-related warnings, please do it > in a separate patch. > > Will do. > > I would like to see more tests for the various cases in this code. > Please make sure there's a test covering every branch you introduced. > Also looking into it. > > -Eli >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
