[cfe-commits] [PATCH] Enhance -Wtautological-compare
Richard Smith
richard at metafoo.co.uk
Thu Mar 1 17:18:06 PST 2012
On Wed, Feb 29, 2012 at 8:36 AM, Xi Wang <xi.wang at gmail.com> wrote:
> Clang doesn't issue warnings against tautological comparisons like
> (uchar < 0) and (uchar == -1). Such tautological comparisons are
> often logic bugs that break the error handling. Below are some
> recent examples.
>
> http://git.kernel.org/linus/3a7f8fb1
> http://git.kernel.org/linus/589665f5
> http://git.kernel.org/linus/4690c33d
>
> This patch enhances Clang to catch such tautological comparisons,
> where bool/uchar/ushort is sign-extended to signed int. Clang
> currently only warns against unsigned tautological comparisons.
>
> diff --git a/include/clang/Basic/DiagnosticSemaKinds.td
b/include/clang/Basic/DiagnosticSemaKinds.td
> index ec4604a..b3c0ae5 100644
> --- a/include/clang/Basic/DiagnosticSemaKinds.td
> +++ b/include/clang/Basic/DiagnosticSemaKinds.td
> @@ -3641,6 +3641,9 @@ def warn_lunsigned_always_true_comparison : Warning<
> def warn_runsigned_always_true_comparison : Warning<
> "comparison of %0 unsigned%select{| enum}2 expression is always %1">,
> InGroup<TautologicalCompare>;
> +def warn_always_true_comparison : Warning<
> + "comparison of %0 %1 %2 is always %select{false|true}3">,
> + InGroup<TautologicalCompare>;
> def warn_comparison_of_mixed_enum_types : Warning<
> "comparison of two values with different enumeration types (%0 and
%1)">,
> InGroup<DiagGroup<"enum-compare">>;
> diff --git a/lib/Sema/SemaChecking.cpp b/lib/Sema/SemaChecking.cpp
> index e963065..025b855 100644
> --- a/lib/Sema/SemaChecking.cpp
> +++ b/lib/Sema/SemaChecking.cpp
> @@ -3664,7 +3664,7 @@ static bool IsSameFloatAfterCast(const APValue
&value,
>
> static void AnalyzeImplicitConversions(Sema &S, Expr *E, SourceLocation
CC);
>
> -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.
> +}
> +
> +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.
> +}
> +
> +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?
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).
> +}
> +
> +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.
> + BinaryOperatorKind Op = E->getOpcode();
> + const char *OpStr = E->getOpcodeStr();
> + Expr *LHS = E->getLHS()->IgnoreParenImpCasts();
> + Expr *RHS = E->getRHS()->IgnoreParenImpCasts();
> + // We are only interested in 'unsigned cmp signed-constant'. Since
> + // the comparison is signed, the unsigned part must be sign-extended.
> + if (E->isEqualityOp()) {
> + llvm::APSInt Value;
> + if (IsUnsigned(LHS) && IsNegative(Value, S, RHS)) {
> + S.Diag(E->getOperatorLoc(), diag::warn_always_true_comparison)
> + << LHS->getType() << OpStr << Value.toString(10) << (Op ==
BO_NE)
> + << E->getLHS()->getSourceRange() <<
E->getRHS()->getSourceRange();
> + } else if (IsNegative(Value, S, LHS) && IsUnsigned(RHS)) {
> + S.Diag(E->getOperatorLoc(), diag::warn_always_true_comparison)
> + << Value.toString(10) << OpStr << RHS->getType() << (Op ==
BO_NE)
> + << E->getLHS()->getSourceRange() <<
E->getRHS()->getSourceRange();
> + }
> + } else if (Op == BO_LT && IsUnsigned(LHS) && IsZero(S, RHS)) {
> + S.Diag(E->getOperatorLoc(), diag::warn_always_true_comparison)
> + << LHS->getType() << OpStr << "0" << 0
> + << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
> + } else if (Op == BO_GE && IsUnsigned(LHS) && IsZero(S, RHS)) {
> + S.Diag(E->getOperatorLoc(), diag::warn_always_true_comparison)
> + << LHS->getType() << OpStr << "0" << 1
> + << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
> + } else if (Op == BO_GT && IsZero(S, LHS) && IsUnsigned(RHS)) {
> + S.Diag(E->getOperatorLoc(), diag::warn_always_true_comparison)
> + << "0" << OpStr << RHS->getType() << 0
> + << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
> + } else if (Op == BO_LE && IsZero(S, LHS) && IsUnsigned(RHS)) {
> + S.Diag(E->getOperatorLoc(), diag::warn_always_true_comparison)
> + << "0" << OpStr << RHS->getType() << 1
> + << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
> + }
> +}
> +
> static void CheckTrivialUnsignedComparison(Sema &S, BinaryOperator *E) {
> BinaryOperatorKind op = E->getOpcode();
> if (E->isValueDependent())
> @@ -3731,14 +3780,19 @@ static void AnalyzeComparison(Sema &S,
BinaryOperator *E) {
> assert(S.Context.hasSameUnqualifiedType(T, E->getRHS()->getType())
> && "comparison with mismatched types");
>
> + // We don't care about value-dependent expressions or expressions
> + // whose result is a constant.
> + if (E->isValueDependent() || E->isIntegerConstantExpr(S.Context))
> + return AnalyzeImpConvsInComparison(S, E);
> +
> + // Check trivial signed comparisons.
> + if (T->hasSignedIntegerRepresentation())
> + CheckTrivialSignedComparison(S, E);
> +
> // We don't do anything special if this isn't an unsigned integral
> // comparison: we're only interested in integral comparisons, and
> // signed comparisons only happen in cases we don't care to warn about.
This comment could do with tweaking.
> - //
> - // We also don't care about value-dependent expressions or expressions
> - // whose result is a constant.
> - if (!T->hasUnsignedIntegerRepresentation()
> - || E->isValueDependent() || E->isIntegerConstantExpr(S.Context))
> + if (!T->hasUnsignedIntegerRepresentation())
> return AnalyzeImpConvsInComparison(S, E);
>
> Expr *LHS = E->getLHS()->IgnoreParenImpCasts();
> diff --git a/test/Sema/compare.c b/test/Sema/compare.c
> index 03aebb3..f340fea 100644
> --- a/test/Sema/compare.c
> +++ b/test/Sema/compare.c
> @@ -284,6 +284,19 @@ int test5(unsigned int x) {
> && (0 <= x); // expected-warning {{comparison of 0 <= unsigned
expression is always true}}
> }
>
> +int uchar(unsigned char x) {
> + return 42
> + + (x < 0) // expected-warning {{comparison of 'unsigned char' < 0
is always false}}
> + + (0 > x) // expected-warning {{comparison of 0 > 'unsigned char'
is always false}}
> + + (x >= 0) // expected-warning {{comparison of 'unsigned char' >=
0 is always true}}
> + + (0 <= x) // expected-warning {{comparison of 0 <= 'unsigned
char' is always true}}
> + + (x == -1) // expected-warning {{comparison of 'unsigned char' ==
-1 is always false}}
> + + (-1 == x) // expected-warning {{comparison of -1 == 'unsigned
char' is always false}}
> + + (x != -1) // expected-warning {{comparison of 'unsigned char' !=
-1 is always true}}
> + + (-1 != x) // expected-warning {{comparison of -1 != 'unsigned
char' is always true}}
> + ;
> +}
> +
> int test6(unsigned i, unsigned power) {
> unsigned x = (i < (1 << power) ? i : 0);
> return x != 3 ? 1 << power : i;
> diff --git a/test/SemaTemplate/instantiate-function-1.cpp
b/test/SemaTemplate/instantiate-function-1.cpp
> index ceef274..3508d88 100644
> --- a/test/SemaTemplate/instantiate-function-1.cpp
> +++ b/test/SemaTemplate/instantiate-function-1.cpp
> @@ -65,7 +65,7 @@ template<typename T, typename U, typename V> struct X6 {
> if (t > 0)
> return u;
> else {
> - if (t < 0)
> + if (t < 0) // expected-warning{{comparison of 'bool' < 0 is
always false}}
> return v; // expected-error{{cannot initialize return object of
type}}
> }
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120301/2836780f/attachment.html>
More information about the cfe-commits
mailing list