[cfe-commits] [PATCH] Enhance -Wtautological-compare

[Xi Wang]

Thanks for the comments!

On Mar 1, 2012, at 8:18 PM, Richard Smith wrote:
> > -static bool IsZero(Sema &S, Expr *E) {
> > +static bool IsIntegerConstant(llvm::APSInt &Value, Sema &S, Expr *E) {
> >    // Suppress cases where we are comparing against an enum constant.
> >    if (const DeclRefExpr *DR =
> >        dyn_cast<DeclRefExpr>(E->IgnoreParenImpCasts()))
> > @@ -3675,8 +3675,16 @@ static bool IsZero(Sema &S, Expr *E) {
> >    if (E->getLocStart().isMacroID())
> >      return false;
> >  
> > +  return E->isIntegerConstantExpr(Value, S.Context);
> 
> I think EvaluateAsInt would be a better call here -- I don't see why it is relevant to this check whether the expression is an integral constant expression. I know this code called isIntegerConstantExpr prior to your change, but if you're changing the code anyway, we may as well fix this.

Sounds great.

> > +}
> > +
> > +static bool IsNegative(llvm::APSInt &Value, Sema &S, Expr *E) {
> > +  return IsIntegerConstant(Value, S, E) && Value.isNegative();
> 
> isNegative just checks the top bit, regardless of whether the integer is signed. Please use Value.isSigned() && Value.isNegative() instead.

I removed Value.isSigned() because I thought it's useless here:
in (uchar == val), uchar is sign-extended, so its top bit is 0;
as long as val's top bit is 1 (i.e., Value.isNegative()), the
comparison is always false.  Adding Value.isSigned() shouldn't
make a difference here.  Or did I miss anything?

> > +static bool IsZero(Sema &S, Expr *E) {
> >    llvm::APSInt Value;
> > -  return E->isIntegerConstantExpr(Value, S.Context) && Value == 0;
> > +  return IsIntegerConstant(Value, S, E) && Value == 0;
> >  }
> >  
> >  static bool HasEnumType(Expr *E) {
> > @@ -3691,6 +3699,47 @@ static bool HasEnumType(Expr *E) {
> >    return E->getType()->isEnumeralType();
> >  }
> >  
> > +static bool IsUnsigned(Expr *E) {
> > +  return E->getType()->isUnsignedIntegerType();
> 
> The existing check also looks for comparisons of unsigned vector types against 0. Are you intentionally excluding those cases from this check?

Do you mean hasUnsignedIntegerRepresentation()?

> Also, both the existing code and your patch don't check for tautological comparisons against unsigned enumeration values in C++ (which don't have unsigned integer type).

Are you aware of any examples of such bugs?  We could add them to
the test case.

> > +static void CheckTrivialSignedComparison(Sema &S, BinaryOperator *E) {
> 
> Would it be possible to merge this with CheckTrivialUnsignedComparison? It seems like that function just checks a special case of what this one checks.

Good idea.  I will give it a try.  So the existing warnings
warn_[lr]unsigned_always_true_comparison that only work for
unsigned comparisons should be replaced, right?

- xi