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

[Richard Trieu]

On Tue, Nov 13, 2012 at 1:50 PM, Eli Friedman <eli.friedman at gmail.com>wrote:

> 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
>

GetExprRange() looks promising for a tighter bound on the bit range.  I
will leave a FIXME to investigate it further.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121113/12ae5072/attachment.html>