[clang-tools-extra] r274731 - [clang-tidy] Enhance redundant-expression check

Etienne Bergeron via cfe-commits cfe-commits at lists.llvm.org
Wed Jul 6 21:03:05 PDT 2016


Author: etienneb
Date: Wed Jul  6 23:03:05 2016
New Revision: 274731

URL: http://llvm.org/viewvc/llvm-project?rev=274731&view=rev
Log:
[clang-tidy] Enhance redundant-expression check

Summary: This patch is adding support to recognize more complex redundant expressions.

Reviewers: alexfh

Subscribers: aaron.ballman, cfe-commits, chrisha

Differential Revision: http://reviews.llvm.org/D21392

Modified:
    clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.cpp
    clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.h
    clang-tools-extra/trunk/test/clang-tidy/misc-redundant-expression.cpp

Modified: clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.cpp?rev=274731&r1=274730&r2=274731&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.cpp Wed Jul  6 23:03:05 2016
@@ -15,14 +15,25 @@
 #include "clang/Lex/Lexer.h"
 
 using namespace clang::ast_matchers;
+using namespace clang::tidy::matchers;
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
+namespace {
+using llvm::APSInt;
+} // namespace
+
 static const char KnownBannedMacroNames[] =
     "EAGAIN;EWOULDBLOCK;SIGCLD;SIGCHLD;";
 
+static bool incrementWithoutOverflow(const APSInt &Value, APSInt &Result) {
+  Result = Value;
+  ++Result;
+  return Value < Result;
+}
+
 static bool areEquivalentNameSpecifier(const NestedNameSpecifier *Left,
                                        const NestedNameSpecifier *Right) {
   llvm::FoldingSetNodeID LeftID, RightID;
@@ -110,6 +121,302 @@ static bool areEquivalentExpr(const Expr
   }
 }
 
+// For a given expression 'x', returns whether the ranges covered by the
+// relational operators are equivalent (i.e.  x <= 4 is equivalent to x < 5).
+static bool areEquivalentRanges(BinaryOperatorKind OpcodeLHS,
+                                const APSInt &ValueLHS,
+                                BinaryOperatorKind OpcodeRHS,
+                                const APSInt &ValueRHS) {
+  assert(APSInt::compareValues(ValueLHS, ValueRHS) <= 0 &&
+         "Values must be ordered");
+  // Handle the case where constants are the same: x <= 4  <==>  x <= 4.
+  if (APSInt::compareValues(ValueLHS, ValueRHS) == 0)
+    return OpcodeLHS == OpcodeRHS;
+
+  // Handle the case where constants are off by one: x <= 4  <==>  x < 5.
+  APSInt ValueLHS_plus1;
+  return ((OpcodeLHS == BO_LE && OpcodeRHS == BO_LT) ||
+          (OpcodeLHS == BO_GT && OpcodeRHS == BO_GE)) &&
+         incrementWithoutOverflow(ValueLHS, ValueLHS_plus1) &&
+         APSInt::compareValues(ValueLHS_plus1, ValueRHS) == 0;
+}
+
+// For a given expression 'x', returns whether the ranges covered by the
+// relational operators are fully disjoint (i.e. x < 4  and  x > 7).
+static bool areExclusiveRanges(BinaryOperatorKind OpcodeLHS,
+                               const APSInt &ValueLHS,
+                               BinaryOperatorKind OpcodeRHS,
+                               const APSInt &ValueRHS) {
+  assert(APSInt::compareValues(ValueLHS, ValueRHS) <= 0 &&
+         "Values must be ordered");
+
+  // Handle cases where the constants are the same.
+  if (APSInt::compareValues(ValueLHS, ValueRHS) == 0) {
+    switch (OpcodeLHS) {
+    case BO_EQ:
+      return OpcodeRHS == BO_NE || OpcodeRHS == BO_GT || OpcodeRHS == BO_LT;
+    case BO_NE:
+      return OpcodeRHS == BO_EQ;
+    case BO_LE:
+      return OpcodeRHS == BO_GT;
+    case BO_GE:
+      return OpcodeRHS == BO_LT;
+    case BO_LT:
+      return OpcodeRHS == BO_EQ || OpcodeRHS == BO_GT || OpcodeRHS == BO_GE;
+    case BO_GT:
+      return OpcodeRHS == BO_EQ || OpcodeRHS == BO_LT || OpcodeRHS == BO_LE;
+    default:
+      return false;
+    }
+  }
+
+  // Handle cases where the constants are different.
+  if ((OpcodeLHS == BO_EQ || OpcodeLHS == BO_LE || OpcodeLHS == BO_LE) &&
+      (OpcodeRHS == BO_EQ || OpcodeRHS == BO_GT || OpcodeRHS == BO_GE))
+    return true;
+
+  // Handle the case where constants are off by one: x > 5 && x < 6.
+  APSInt ValueLHS_plus1;
+  if (OpcodeLHS == BO_GT && OpcodeRHS == BO_LT &&
+      incrementWithoutOverflow(ValueLHS, ValueLHS_plus1) &&
+      APSInt::compareValues(ValueLHS_plus1, ValueRHS) == 0)
+    return true;
+
+  return false;
+}
+
+// Returns whether the ranges covered by the union of both relational
+// expressions covers the whole domain (i.e. x < 10  and  x > 0).
+static bool rangesFullyCoverDomain(BinaryOperatorKind OpcodeLHS,
+                                   const APSInt &ValueLHS,
+                                   BinaryOperatorKind OpcodeRHS,
+                                   const APSInt &ValueRHS) {
+  assert(APSInt::compareValues(ValueLHS, ValueRHS) <= 0 &&
+         "Values must be ordered");
+
+  // Handle cases where the constants are the same:  x < 5 || x >= 5.
+  if (APSInt::compareValues(ValueLHS, ValueRHS) == 0) {
+    switch (OpcodeLHS) {
+    case BO_EQ:
+      return OpcodeRHS == BO_NE;
+    case BO_NE:
+      return OpcodeRHS == BO_EQ;
+    case BO_LE:
+      return OpcodeRHS == BO_GT || OpcodeRHS == BO_GE;
+    case BO_LT:
+      return OpcodeRHS == BO_GE;
+    case BO_GE:
+      return OpcodeRHS == BO_LT || OpcodeRHS == BO_LE;
+    case BO_GT:
+      return OpcodeRHS == BO_LE;
+    default:
+      return false;
+    }
+  }
+
+  // Handle the case where constants are off by one: x <= 4 || x >= 5.
+  APSInt ValueLHS_plus1;
+  if (OpcodeLHS == BO_LE && OpcodeRHS == BO_GE &&
+      incrementWithoutOverflow(ValueLHS, ValueLHS_plus1) &&
+      APSInt::compareValues(ValueLHS_plus1, ValueRHS) == 0)
+    return true;
+
+  // Handle cases where the constants are different: x > 4 || x <= 7.
+  if ((OpcodeLHS == BO_GT || OpcodeLHS == BO_GE) &&
+      (OpcodeRHS == BO_LT || OpcodeRHS == BO_LE))
+    return true;
+
+  return false;
+}
+
+static bool rangeSubsumesRange(BinaryOperatorKind OpcodeLHS,
+                               const APSInt &ValueLHS,
+                               BinaryOperatorKind OpcodeRHS,
+                               const APSInt &ValueRHS) {
+  int Comparison = APSInt::compareValues(ValueLHS, ValueRHS);
+  switch (OpcodeLHS) {
+  case BO_EQ:
+    return OpcodeRHS == BO_EQ && Comparison == 0;
+  case BO_NE:
+    return (OpcodeRHS == BO_NE && Comparison == 0) ||
+           (OpcodeRHS == BO_EQ && Comparison != 0) ||
+           (OpcodeRHS == BO_LT && Comparison >= 0) ||
+           (OpcodeRHS == BO_LE && Comparison > 0) ||
+           (OpcodeRHS == BO_GT && Comparison <= 0) ||
+           (OpcodeRHS == BO_GE && Comparison < 0);
+
+  case BO_LT:
+    return ((OpcodeRHS == BO_LT && Comparison >= 0) ||
+            (OpcodeRHS == BO_LE && Comparison > 0) ||
+            (OpcodeRHS == BO_EQ && Comparison > 0));
+  case BO_GT:
+    return ((OpcodeRHS == BO_GT && Comparison <= 0) ||
+            (OpcodeRHS == BO_GE && Comparison < 0) ||
+            (OpcodeRHS == BO_EQ && Comparison < 0));
+  case BO_LE:
+    return (OpcodeRHS == BO_LT || OpcodeRHS == BO_LE || OpcodeRHS == BO_EQ) &&
+           Comparison >= 0;
+  case BO_GE:
+    return (OpcodeRHS == BO_GT || OpcodeRHS == BO_GE || OpcodeRHS == BO_EQ) &&
+           Comparison <= 0;
+  default:
+    return false;
+  }
+}
+
+static void canonicalNegateExpr(BinaryOperatorKind &Opcode, APSInt &Value) {
+  if (Opcode == BO_Sub) {
+    Opcode = BO_Add;
+    Value = -Value;
+  }
+}
+
+AST_MATCHER(Expr, isIntegerConstantExpr) {
+  if (Node.isInstantiationDependent())
+    return false;
+  return Node.isIntegerConstantExpr(Finder->getASTContext());
+}
+
+// Returns a matcher for integer constant expression.
+static ast_matchers::internal::Matcher<Expr>
+matchIntegerConstantExpr(StringRef Id) {
+  std::string CstId = (Id + "-const").str();
+  return expr(isIntegerConstantExpr()).bind(CstId);
+}
+
+// Retrieve the integer value matched by 'matchIntegerConstantExpr' with name
+// 'Id' and store it into 'Value'.
+static bool retrieveIntegerConstantExpr(const MatchFinder::MatchResult &Result,
+                                        StringRef Id, APSInt &Value) {
+  std::string CstId = (Id + "-const").str();
+  const auto *CstExpr = Result.Nodes.getNodeAs<Expr>(CstId);
+  return CstExpr && CstExpr->isIntegerConstantExpr(Value, *Result.Context);
+}
+
+// Returns a matcher for a symbolic expression (any expression except ingeter
+// constant expression).
+static ast_matchers::internal::Matcher<Expr> matchSymbolicExpr(StringRef Id) {
+  std::string SymId = (Id + "-sym").str();
+  return ignoringParenImpCasts(
+      expr(unless(isIntegerConstantExpr())).bind(SymId));
+}
+
+// Retrieve the expression matched by 'matchSymbolicExpr' with name 'Id' and
+// store it into 'SymExpr'.
+static bool retrieveSymbolicExpr(const MatchFinder::MatchResult &Result,
+                                 StringRef Id, const Expr *&SymExpr) {
+  std::string SymId = (Id + "-sym").str();
+  if (const auto *Node = Result.Nodes.getNodeAs<Expr>(SymId)) {
+    SymExpr = Node;
+    return true;
+  }
+  return false;
+}
+
+// Match a binary operator between a symbolic expression and an integer constant
+// expression.
+static ast_matchers::internal::Matcher<Expr>
+matchBinOpIntegerConstantExpr(StringRef Id) {
+  const auto BinOpCstExpr =
+      expr(
+          anyOf(binaryOperator(anyOf(hasOperatorName("+"), hasOperatorName("|"),
+                                     hasOperatorName("&")),
+                               hasEitherOperand(matchSymbolicExpr(Id)),
+                               hasEitherOperand(matchIntegerConstantExpr(Id))),
+                binaryOperator(hasOperatorName("-"),
+                               hasLHS(matchSymbolicExpr(Id)),
+                               hasRHS(matchIntegerConstantExpr(Id)))))
+          .bind(Id);
+  return ignoringParenImpCasts(BinOpCstExpr);
+}
+
+// Retrieve sub-expressions matched by 'matchBinOpIntegerConstantExpr' with
+// name 'Id'.
+static bool
+retrieveBinOpIntegerConstantExpr(const MatchFinder::MatchResult &Result,
+                                 StringRef Id, BinaryOperatorKind &Opcode,
+                                 const Expr *&Symbol, APSInt &Value) {
+  if (const auto *BinExpr = Result.Nodes.getNodeAs<BinaryOperator>(Id)) {
+    Opcode = BinExpr->getOpcode();
+    return retrieveSymbolicExpr(Result, Id, Symbol) &&
+           retrieveIntegerConstantExpr(Result, Id, Value);
+  }
+  return false;
+}
+
+// Matches relational expression: 'Expr <op> k' (i.e. x < 2, x != 3, 12 <= x).
+static ast_matchers::internal::Matcher<Expr>
+matchRelationalIntegerConstantExpr(StringRef Id) {
+  std::string CastId = (Id + "-cast").str();
+  std::string SwapId = (Id + "-swap").str();
+  std::string NegateId = (Id + "-negate").str();
+
+  const auto RelationalExpr = ignoringParenImpCasts(binaryOperator(
+      isComparisonOperator(), expr().bind(Id),
+      anyOf(allOf(hasLHS(matchSymbolicExpr(Id)),
+                  hasRHS(matchIntegerConstantExpr(Id))),
+            allOf(hasLHS(matchIntegerConstantExpr(Id)),
+                  hasRHS(matchSymbolicExpr(Id)), expr().bind(SwapId)))));
+
+  // A cast can be matched as a comparator to zero. (i.e. if (x) is equivalent
+  // to if (x != 0)).
+  const auto CastExpr =
+      implicitCastExpr(hasCastKind(CK_IntegralToBoolean),
+                       hasSourceExpression(matchSymbolicExpr(Id)))
+          .bind(CastId);
+
+  const auto NegateRelationalExpr =
+      unaryOperator(hasOperatorName("!"),
+                    hasUnaryOperand(anyOf(CastExpr, RelationalExpr)))
+          .bind(NegateId);
+
+  const auto NegateNegateRelationalExpr =
+      unaryOperator(hasOperatorName("!"),
+                    hasUnaryOperand(unaryOperator(
+                        hasOperatorName("!"),
+                        hasUnaryOperand(anyOf(CastExpr, RelationalExpr)))));
+
+  return anyOf(RelationalExpr, CastExpr, NegateRelationalExpr,
+               NegateNegateRelationalExpr);
+}
+
+// Retrieve sub-expressions matched by 'matchRelationalIntegerConstantExpr' with
+// name 'Id'.
+static bool
+retrieveRelationalIntegerConstantExpr(const MatchFinder::MatchResult &Result,
+                                      StringRef Id, const Expr *&OperandExpr,
+                                      BinaryOperatorKind &Opcode,
+                                      const Expr *&Symbol, APSInt &Value) {
+  std::string CastId = (Id + "-cast").str();
+  std::string SwapId = (Id + "-swap").str();
+  std::string NegateId = (Id + "-negate").str();
+
+  if (const auto *Bin = Result.Nodes.getNodeAs<BinaryOperator>(Id)) {
+    // Operand received with explicit comparator.
+    Opcode = Bin->getOpcode();
+    OperandExpr = Bin;
+    if (!retrieveIntegerConstantExpr(Result, Id, Value))
+      return false;
+  } else if (const auto *Cast = Result.Nodes.getNodeAs<CastExpr>(CastId)) {
+    // Operand received with implicit comparator (cast).
+    Opcode = BO_NE;
+    OperandExpr = Cast;
+    Value = APSInt(32, 0);
+  } else {
+    return false;
+  }
+
+  if (!retrieveSymbolicExpr(Result, Id, Symbol))
+    return false;
+
+  if (Result.Nodes.getNodeAs<Expr>(SwapId))
+    Opcode = BinaryOperator::reverseComparisonOp(Opcode);
+  if (Result.Nodes.getNodeAs<Expr>(NegateId))
+    Opcode = BinaryOperator::negateComparisonOp(Opcode);
+
+  return true;
+}
+
 AST_MATCHER(BinaryOperator, operandsAreEquivalent) {
   return areEquivalentExpr(Node.getLHS(), Node.getRHS());
 }
@@ -199,6 +506,206 @@ void RedundantExpressionCheck::registerM
           unless(isMacro()), unless(isInTemplateInstantiation()))
           .bind("call"),
       this);
+
+  // Match common expressions and apply more checks to find redundant
+  // sub-expressions.
+  //   a) Expr <op> K1 == K2
+  //   b) Expr <op> K1 == Expr
+  //   c) Expr <op> K1 == Expr <op> K2
+  // see: 'checkArithmeticExpr' and 'checkBitwiseExpr'
+  const auto BinOpCstLeft = matchBinOpIntegerConstantExpr("lhs");
+  const auto BinOpCstRight = matchBinOpIntegerConstantExpr("rhs");
+  const auto CstRight = matchIntegerConstantExpr("rhs");
+  const auto SymRight = matchSymbolicExpr("rhs");
+
+  // Match expressions like: x <op> 0xFF == 0xF00.
+  Finder->addMatcher(binaryOperator(isComparisonOperator(),
+                                    hasEitherOperand(BinOpCstLeft),
+                                    hasEitherOperand(CstRight))
+                         .bind("binop-const-compare-to-const"),
+                     this);
+
+  // Match expressions like: x <op> 0xFF == x.
+  Finder->addMatcher(
+      binaryOperator(isComparisonOperator(),
+                     anyOf(allOf(hasLHS(BinOpCstLeft), hasRHS(SymRight)),
+                           allOf(hasLHS(SymRight), hasRHS(BinOpCstLeft))))
+          .bind("binop-const-compare-to-sym"),
+      this);
+
+  // Match expressions like: x <op> 10 == x <op> 12.
+  Finder->addMatcher(binaryOperator(isComparisonOperator(),
+                                    hasLHS(BinOpCstLeft), hasRHS(BinOpCstRight),
+                                    // Already reported as redundant.
+                                    unless(operandsAreEquivalent()))
+                         .bind("binop-const-compare-to-binop-const"),
+                     this);
+
+  // Match relational expressions combined with logical operators and find
+  // redundant sub-expressions.
+  // see: 'checkRelationalExpr'
+
+  // Match expressions like: x < 2 && x > 2.
+  const auto ComparisonLeft = matchRelationalIntegerConstantExpr("lhs");
+  const auto ComparisonRight = matchRelationalIntegerConstantExpr("rhs");
+  Finder->addMatcher(
+      binaryOperator(anyOf(hasOperatorName("||"), hasOperatorName("&&")),
+                     hasLHS(ComparisonLeft), hasRHS(ComparisonRight),
+                     // Already reported as redundant.
+                     unless(operandsAreEquivalent()))
+          .bind("comparisons-of-symbol-and-const"),
+      this);
+}
+
+void RedundantExpressionCheck::checkArithmeticExpr(
+    const MatchFinder::MatchResult &Result) {
+  APSInt LhsValue, RhsValue;
+  const Expr *LhsSymbol = nullptr, *RhsSymbol = nullptr;
+  BinaryOperatorKind LhsOpcode, RhsOpcode;
+
+  if (const auto *ComparisonOperator = Result.Nodes.getNodeAs<BinaryOperator>(
+          "binop-const-compare-to-sym")) {
+    BinaryOperatorKind Opcode = ComparisonOperator->getOpcode();
+    if (!retrieveBinOpIntegerConstantExpr(Result, "lhs", LhsOpcode, LhsSymbol,
+                                          LhsValue) ||
+        !retrieveSymbolicExpr(Result, "rhs", RhsSymbol) ||
+        !areEquivalentExpr(LhsSymbol, RhsSymbol))
+      return;
+
+    // Check expressions: x + k == x  or  x - k == x.
+    if (LhsOpcode == BO_Add || LhsOpcode == BO_Sub) {
+      if ((LhsValue != 0 && Opcode == BO_EQ) ||
+          (LhsValue == 0 && Opcode == BO_NE))
+        diag(ComparisonOperator->getOperatorLoc(),
+             "logical expression is always false");
+      else if ((LhsValue == 0 && Opcode == BO_EQ) ||
+               (LhsValue != 0 && Opcode == BO_NE))
+        diag(ComparisonOperator->getOperatorLoc(),
+             "logical expression is always true");
+    }
+  } else if (const auto *ComparisonOperator =
+                 Result.Nodes.getNodeAs<BinaryOperator>(
+                     "binop-const-compare-to-binop-const")) {
+    BinaryOperatorKind Opcode = ComparisonOperator->getOpcode();
+
+    if (!retrieveBinOpIntegerConstantExpr(Result, "lhs", LhsOpcode, LhsSymbol,
+                                          LhsValue) ||
+        !retrieveBinOpIntegerConstantExpr(Result, "rhs", RhsOpcode, RhsSymbol,
+                                          RhsValue) ||
+        !areEquivalentExpr(LhsSymbol, RhsSymbol))
+      return;
+
+    canonicalNegateExpr(LhsOpcode, LhsValue);
+    canonicalNegateExpr(RhsOpcode, RhsValue);
+
+    // Check expressions: x + 1 == x + 2  or  x + 1 != x + 2.
+    if (LhsOpcode == BO_Add && RhsOpcode == BO_Add) {
+      if ((Opcode == BO_EQ && APSInt::compareValues(LhsValue, RhsValue) == 0) ||
+          (Opcode == BO_NE && APSInt::compareValues(LhsValue, RhsValue) != 0)) {
+        diag(ComparisonOperator->getOperatorLoc(),
+             "logical expression is always true");
+      } else if ((Opcode == BO_EQ &&
+                  APSInt::compareValues(LhsValue, RhsValue) != 0) ||
+                 (Opcode == BO_NE &&
+                  APSInt::compareValues(LhsValue, RhsValue) == 0)) {
+        diag(ComparisonOperator->getOperatorLoc(),
+             "logical expression is always false");
+      }
+    }
+  }
+}
+
+void RedundantExpressionCheck::checkBitwiseExpr(
+    const MatchFinder::MatchResult &Result) {
+  if (const auto *ComparisonOperator = Result.Nodes.getNodeAs<BinaryOperator>(
+          "binop-const-compare-to-const")) {
+    BinaryOperatorKind Opcode = ComparisonOperator->getOpcode();
+
+    APSInt LhsValue, RhsValue;
+    const Expr *LhsSymbol = nullptr;
+    BinaryOperatorKind LhsOpcode;
+    if (!retrieveBinOpIntegerConstantExpr(Result, "lhs", LhsOpcode, LhsSymbol,
+                                          LhsValue) ||
+        !retrieveIntegerConstantExpr(Result, "rhs", RhsValue))
+      return;
+
+    uint64_t LhsConstant = LhsValue.getZExtValue();
+    uint64_t RhsConstant = RhsValue.getZExtValue();
+    SourceLocation Loc = ComparisonOperator->getOperatorLoc();
+
+    // Check expression: x & k1 == k2  (i.e. x & 0xFF == 0xF00)
+    if (LhsOpcode == BO_And && (LhsConstant & RhsConstant) != RhsConstant) {
+      if (Opcode == BO_EQ)
+        diag(Loc, "logical expression is always false");
+      else if (Opcode == BO_NE)
+        diag(Loc, "logical expression is always true");
+    }
+
+    // Check expression: x | k1 == k2  (i.e. x | 0xFF == 0xF00)
+    if (LhsOpcode == BO_Or && (LhsConstant | RhsConstant) != RhsConstant) {
+      if (Opcode == BO_EQ)
+        diag(Loc, "logical expression is always false");
+      else if (Opcode == BO_NE)
+        diag(Loc, "logical expression is always true");
+    }
+  }
+}
+
+void RedundantExpressionCheck::checkRelationalExpr(
+    const MatchFinder::MatchResult &Result) {
+  if (const auto *ComparisonOperator = Result.Nodes.getNodeAs<BinaryOperator>(
+          "comparisons-of-symbol-and-const")) {
+    // Matched expressions are: (x <op> k1) <REL> (x <op> k2).
+    BinaryOperatorKind Opcode = ComparisonOperator->getOpcode();
+
+    const Expr *LhsExpr = nullptr, *RhsExpr = nullptr;
+    APSInt LhsValue, RhsValue;
+    const Expr *LhsSymbol = nullptr, *RhsSymbol = nullptr;
+    BinaryOperatorKind LhsOpcode, RhsOpcode;
+    if (!retrieveRelationalIntegerConstantExpr(
+            Result, "lhs", LhsExpr, LhsOpcode, LhsSymbol, LhsValue) ||
+        !retrieveRelationalIntegerConstantExpr(
+            Result, "rhs", RhsExpr, RhsOpcode, RhsSymbol, RhsValue) ||
+        !areEquivalentExpr(LhsSymbol, RhsSymbol))
+      return;
+
+    // Bring to a canonical form: smallest constant must be on the left side.
+    if (APSInt::compareValues(LhsValue, RhsValue) > 0) {
+      std::swap(LhsExpr, RhsExpr);
+      std::swap(LhsValue, RhsValue);
+      std::swap(LhsSymbol, RhsSymbol);
+      std::swap(LhsOpcode, RhsOpcode);
+    }
+
+    if ((Opcode == BO_LAnd || Opcode == BO_LOr) &&
+        areEquivalentRanges(LhsOpcode, LhsValue, RhsOpcode, RhsValue)) {
+      diag(ComparisonOperator->getOperatorLoc(),
+           "equivalent expression on both side of logical operator");
+      return;
+    }
+
+    if (Opcode == BO_LAnd) {
+      if (areExclusiveRanges(LhsOpcode, LhsValue, RhsOpcode, RhsValue)) {
+        diag(ComparisonOperator->getOperatorLoc(),
+             "logical expression is always false");
+      } else if (rangeSubsumesRange(LhsOpcode, LhsValue, RhsOpcode, RhsValue)) {
+        diag(LhsExpr->getExprLoc(), "expression is redundant");
+      } else if (rangeSubsumesRange(RhsOpcode, RhsValue, LhsOpcode, LhsValue)) {
+        diag(RhsExpr->getExprLoc(), "expression is redundant");
+      }
+    }
+
+    if (Opcode == BO_LOr) {
+      if (rangesFullyCoverDomain(LhsOpcode, LhsValue, RhsOpcode, RhsValue)) {
+        diag(ComparisonOperator->getOperatorLoc(),
+             "logical expression is always true");
+      } else if (rangeSubsumesRange(LhsOpcode, LhsValue, RhsOpcode, RhsValue)) {
+        diag(RhsExpr->getExprLoc(), "expression is redundant");
+      } else if (rangeSubsumesRange(RhsOpcode, RhsValue, LhsOpcode, LhsValue)) {
+        diag(LhsExpr->getExprLoc(), "expression is redundant");
+      }
+    }
+  }
 }
 
 void RedundantExpressionCheck::check(const MatchFinder::MatchResult &Result) {
@@ -209,6 +716,10 @@ void RedundantExpressionCheck::check(con
   if (const auto *Call = Result.Nodes.getNodeAs<CXXOperatorCallExpr>("call"))
     diag(Call->getOperatorLoc(),
          "both side of overloaded operator are equivalent");
+
+  checkArithmeticExpr(Result);
+  checkBitwiseExpr(Result);
+  checkRelationalExpr(Result);
 }
 
 } // namespace misc

Modified: clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.h?rev=274731&r1=274730&r2=274731&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.h (original)
+++ clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.h Wed Jul  6 23:03:05 2016
@@ -26,6 +26,11 @@ public:
       : ClangTidyCheck(Name, Context) {}
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  void checkArithmeticExpr(const ast_matchers::MatchFinder::MatchResult &R);
+  void checkBitwiseExpr(const ast_matchers::MatchFinder::MatchResult &R);
+  void checkRelationalExpr(const ast_matchers::MatchFinder::MatchResult &R);
 };
 
 } // namespace misc

Modified: clang-tools-extra/trunk/test/clang-tidy/misc-redundant-expression.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/misc-redundant-expression.cpp?rev=274731&r1=274730&r2=274731&view=diff
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/misc-redundant-expression.cpp (original)
+++ clang-tools-extra/trunk/test/clang-tidy/misc-redundant-expression.cpp Wed Jul  6 23:03:05 2016
@@ -1,4 +1,6 @@
-// RUN: %check_clang_tidy %s misc-redundant-expression %t
+// RUN: %check_clang_tidy %s misc-redundant-expression %t -- -- -std=c++11
+
+typedef __INT64_TYPE__ I64;
 
 struct Point {
   int x;
@@ -64,7 +66,7 @@ int Test(int X, int Y) {
 
   if ( + "dummy" == + "dummy") return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: both side of operator are equivalent
-  if (L"abc" == L"abc") return 1;     
+  if (L"abc" == L"abc") return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both side of operator are equivalent
 
   if (foo(0) - 2 < foo(0) - 2) return 1;
@@ -82,7 +84,7 @@ int Test(int X, int Y) {
 
 int Valid(int X, int Y) {
   if (X != Y) return 1;
-  if (X == X + 0) return 1;
+  if (X == Y + 0) return 1;
   if (P.x == P.y) return 1;
   if (P.a[P.x] < P.a[P.y]) return 1;
   if (P.a[0] < P.a[1]) return 1;
@@ -160,3 +162,324 @@ void TemplateCheck() {
   // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: both side of overloaded operator are equivalent
 }
 void TestTemplate() { TemplateCheck<MyClass, MyClass>(); }
+
+int TestArithmetic(int X, int Y) {
+  if (X + 1 == X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
+  if (X + 1 != X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+  if (X - 1 == X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
+  if (X - 1 != X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+
+  if (X + 1LL == X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always false
+  if (X + 1ULL == X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: logical expression is always false
+
+  if (X == X + 1) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always false
+  if (X != X + 1) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always true
+  if (X == X - 1) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always false
+  if (X != X - 1) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always true
+
+  if (X != X - 1U) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always true
+  if (X != X - 1LL) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always true
+
+  if ((X+X) != (X+X) - 1) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+
+  if (X + 1 == X + 2) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
+  if (X + 1 != X + 2) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+
+  if (X - 1 == X - 2) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
+  if (X - 1 != X - 2) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+
+  if (X + 1 == X - -1) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+  if (X + 1 != X - -1) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
+  if (X + 1 == X - -2) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
+  if (X + 1 != X - -2) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+
+  if (X + 1 == X - (~0)) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+  if (X + 1 == X - (~0U)) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+  if (X + 1 == X - (~0ULL)) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+
+  // Should not match.
+  if (X + 0.5 == X) return 1;
+  if (X + 1 == Y) return 1;
+  if (X + 1 == Y + 1) return 1;
+  if (X + 1 == Y + 2) return 1;
+
+  return 0;
+}
+
+int TestBitwise(int X) {
+  if ((X & 0xFF) == 0xF00) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always false
+  if ((X & 0xFF) != 0xF00) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always true
+  if ((X | 0xFF) == 0xF00) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always false
+  if ((X | 0xFF) != 0xF00) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always true
+
+  if ((X | 0xFFULL) != 0xF00) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: logical expression is always true
+  if ((X | 0xFF) != 0xF00ULL) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always true
+
+  if ((0xFF & X) == 0xF00) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always false
+  if ((0xFF & X) != 0xF00) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always true
+  if ((0xFF & X) == 0xF00) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always false
+  if ((0xFF & X) != 0xF00) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always true
+
+  if ((0xFFLL & X) == 0xF00) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: logical expression is always false
+  if ((0xFF & X) == 0xF00ULL) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always false
+
+  return 0;
+}
+
+int TestRelational(int X, int Y) {
+  if (X == 10 && X != 10) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always false
+  if (X == 10 && (X != 10)) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always false
+  if (X == 10 && !(X == 10)) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always false
+  if (!(X != 10) && !(X == 10)) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always false
+
+  if (X == 10ULL && X != 10ULL) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always false
+  if (!(X != 10U) && !(X == 10)) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: logical expression is always false
+  if (!(X != 10LL) && !(X == 10)) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: logical expression is always false
+  if (!(X != 10ULL) && !(X == 10)) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: logical expression is always false
+
+  if (X == 0 && X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: logical expression is always false
+  if (X != 0 && !X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: logical expression is always false
+  if (X && !X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always false
+
+  if (X && !!X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: equivalent expression on both side of logical operator
+  if (X != 0 && X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: equivalent expression on both side of logical operator
+  if (X != 0 && !!X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: equivalent expression on both side of logical operator
+  if (X == 0 && !X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: equivalent expression on both side of logical operator
+
+  if (X == 10 && X > 10) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always false
+  if (X == 10 && X < 10) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always false
+  if (X < 10 && X > 10) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: logical expression is always false
+  if (X <= 10 && X > 10) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always false
+  if (X < 10 && X >= 10) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: logical expression is always false
+  if (X < 10 && X == 10) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: logical expression is always false
+
+  if (X > 5 && X <= 5) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
+  if (X > -5 && X <= -5) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: logical expression is always false
+
+  if (X < 10 || X >= 10) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: logical expression is always true
+  if (X <= 10 || X > 10) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always true
+  if (X <= 10 || X >= 11) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always true
+
+  if (X < 7 && X < 6) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant
+  if (X < 7 && X < 7) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent
+  if (X < 7 && X < 8) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: expression is redundant
+
+  if (X < 7 && X <= 5) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant 
+  if (X < 7 && X <= 6) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: equivalent expression on both side of logical operator
+  if (X < 7 && X <= 7) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: expression is redundant
+  if (X < 7 && X <= 8) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: expression is redundant
+
+  if (X <= 7 && X < 6) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant
+  if (X <= 7 && X < 7) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant
+  if (X <= 7 && X < 8) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: equivalent expression on both side of logical operator
+
+  if (X <= 7 && X <= 5) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant
+  if (X <= 7 && X <= 6) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant
+  if (X <= 7 && X <= 7) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both side of operator are equivalent
+  if (X <= 7 && X <= 8) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: expression is redundant 
+
+  if (X == 11 && X > 10) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: expression is redundant 
+  if (X == 11 && X < 12) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: expression is redundant 
+  if (X > 10 && X == 11) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant 
+  if (X < 12 && X == 11) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant 
+ 
+  if (X != 11 && X == 42) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant 
+  if (X != 11 && X > 11) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant 
+  if (X != 11 && X < 11) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant 
+  if (X != 11 && X < 8) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant 
+  if (X != 11 && X > 14) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant 
+
+  if (X < 7 || X < 6) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: expression is redundant
+  if (X < 7 || X < 7) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent
+  if (X < 7 || X < 8) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant
+
+  // Should not match.
+  if (X == 10 && Y == 10) return 1;
+  if (X != 10 && X != 12) return 1;
+  if (X == 10 || X == 12) return 1;
+  if (X < 10 || X > 12) return 1;
+  if (X > 10 && X < 12) return 1;
+  if (X < 10 || X >= 12) return 1;
+  if (X > 10 && X <= 12) return 1;
+  if (X <= 10 || X > 12) return 1;
+  if (X >= 10 && X < 12) return 1;
+  if (X <= 10 || X >= 12) return 1;
+  if (X >= 10 && X <= 12) return 1;
+  if (X >= 10 && X <= 11) return 1;
+  if (X >= 10 && X < 11) return 1;
+  if (X > 10 && X <= 11) return 1;
+  if (X > 10 && X != 11) return 1;
+  if (X >= 10 && X <= 10) return 1;
+  if (X <= 10 && X >= 10) return 1;
+  if (!X && !Y) return 1;
+  if (!X && Y) return 1;
+  if (!X && Y == 0) return 1;
+  if (X == 10 && Y != 10) return 1;
+  if (X < 0 || X > 0) return 1;
+
+  return 0;
+}
+
+int TestValidExpression(int X) {
+  if (X - 1 == 1 - X) return 1;
+  if (2 * X == X) return 1;
+  if ((X << 1) == X) return 1;
+
+  return 0;
+}
+
+enum Color { Red, Yellow, Green };
+int TestRelatiopnalWithEnum(enum Color C) {
+  if (C == Red && C == Yellow) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: logical expression is always false
+  if (C == Red && C != Red) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: logical expression is always false
+
+  // Should not match.
+  if (C == Red || C == Yellow) return 1;
+  if (C != Red && C != Yellow) return 1;
+
+  return 0;
+}
+
+template<class T>
+int TestRelationalTemplated(int X) {
+  // This test causes a corner case with |isIntegerConstantExpr| where the type
+  // is dependant. There is an assert failing when evaluating
+  // sizeof(<incomplet-type>).
+  if (sizeof(T) == 4 || sizeof(T) == 8) return 1;
+
+  if (X + 0 == -X) return 1;
+  if (X + 0 < X) return 1;
+
+  return 0;
+}
+
+int TestWithSignedUnsigned(int X) {
+  if (X + 1 == X + 1ULL) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+  if ((X & 0xFFU) == 0xF00) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: logical expression is always false
+  if ((X & 0xFF) == 0xF00U) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always false
+  if ((X & 0xFFU) == 0xF00U) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: logical expression is always false
+
+  return 0;
+}
+
+int TestWithLong(int X, I64 Y) {
+  if (X + 0ULL == -X) return 1;
+  if (Y + 0 == -Y) return 1;
+  if (Y <= 10 && X >= 10LL) return 1;
+  if (Y <= 10 && X >= 10ULL) return 1;
+  if (X <= 10 || X > 12LL) return 1;
+  if (X <= 10 || X > 12ULL) return 1;
+  if (Y <= 10 || Y > 12) return 1;
+
+  return 0;
+}
+
+int TestWithMinMaxInt(int X) {
+  if (X <= X + 0xFFFFFFFFU) return 1;
+  if (X <= X + 0x7FFFFFFF) return 1;
+  if (X <= X + 0x80000000) return 1;
+  if (X <= 0xFFFFFFFFU && X > 0) return 1;
+  if (X <= 0xFFFFFFFFU && X > 0U) return 1;
+
+  if (X + 0x80000000 == X - 0x80000000) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: logical expression is always true
+
+  if (X > 0x7FFFFFFF || X < ((-0x7FFFFFFF)-1)) return 1;
+  if (X <= 0x7FFFFFFF && X >= ((-0x7FFFFFFF)-1)) return 1;
+  
+  return 0;
+}




More information about the cfe-commits mailing list