[cfe-commits] [PATCH] Improve -Wtautological-constant-out-of-range-compare
Eli Friedman
eli.friedman at gmail.com
Tue Nov 13 13:50:35 PST 2012
On Tue, Nov 13, 2012 at 1:35 PM, Richard Trieu <rtrieu at google.com> wrote:
> On Mon, Nov 12, 2012 at 6:35 PM, Eli Friedman <eli.friedman at gmail.com>
> wrote:
>>
>> On Mon, Nov 12, 2012 at 5:31 PM, Richard Trieu <rtrieu at google.com> wrote:
>> > I uploaded a new patch.
>> >
>> > On Mon, Nov 12, 2012 at 11:22 AM, Eli Friedman <eli.friedman at gmail.com>
>> > wrote:
>> >>
>> >> Comments below.
>> >>
>> >> On Mon, Nov 12, 2012 at 9:46 AM, Richard Trieu <rtrieu at google.com>
>> >> 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
More information about the cfe-commits
mailing list