On Tue, Nov 13, 2012 at 1:35 PM, Richard Trieu <[email protected]> wrote: > 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().
Oh! I was assuming the code was using the GetExprRange() function, but it looks like it isn't for reasons I don't really understand. -Eli _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
