[cfe-commits] [PATCH] Improve -Wtautological-constant-out-of-range-compare

[Eli Friedman]

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.

Again, if you want to change our enum-related warnings, please do it
in a separate patch.


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.

-Eli