[clang-tidy] New checker for redundant expressions.

Summary:
This checker finds redundant expression on both side of a binary operator.

The current implementation provide a function to check whether expressions
are equivalent. This implementation is able to recognize the common
subset encounter in C++ program. Side-effects like "x++" are not considered
to be equivalent.

There are many False Positives related to macros and to floating point
computations (detecting NaN). The checker is ignoring these cases.

Example:
```
    if( !dst || dst->depth != desired_depth ||
        dst->nChannels != desired_num_channels ||
        dst_size.width != src_size.width ||
        dst_size.height != dst_size.height )    <<--- bug
    {
```

Reviewers: alexfh

Subscribers: danielmarjamaki, fahlgren, jordan_rose, zaks.anna, Eugene.Zelenko, cfe-commits

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

llvm-svn: 267574

clang-tools-extra/clang-tidy/misc/CMakeLists.txt

@@ -23,6 +23,7 @@ add_clang_library(clangTidyMiscModule
   NoexceptMoveConstructorCheck.cpp
   NonCopyableObjects.cpp
   PointerAndIntegralOperationCheck.cpp
+  RedundantExpressionCheck.cpp
   SizeofContainerCheck.cpp
   SizeofExpressionCheck.cpp
   StaticAssertCheck.cpp

clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp

@@ -31,6 +31,7 @@
 #include "NoexceptMoveConstructorCheck.h"
 #include "NonCopyableObjects.h"
 #include "PointerAndIntegralOperationCheck.h"
+#include "RedundantExpressionCheck.h"
 #include "SizeofContainerCheck.h"
 #include "SizeofExpressionCheck.h"
 #include "StaticAssertCheck.h"
@@ -98,6 +99,8 @@ class MiscModule : public ClangTidyModule {
         "misc-non-copyable-objects");
     CheckFactories.registerCheck<PointerAndIntegralOperationCheck>(
         "misc-pointer-and-integral-operation");
+    CheckFactories.registerCheck<RedundantExpressionCheck>(
+        "misc-redundant-expression");
     CheckFactories.registerCheck<SizeofContainerCheck>("misc-sizeof-container");
     CheckFactories.registerCheck<SizeofExpressionCheck>(
         "misc-sizeof-expression");

clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp

@@ -0,0 +1,133 @@
+//===--- RedundantExpressionCheck.cpp - clang-tidy-------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "RedundantExpressionCheck.h"
+#include "../utils/Matchers.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+static bool AreIdenticalExpr(const Expr *Left, const Expr *Right) {
+  if (!Left || !Right)
+    return !Left && !Right;
+
+  Left = Left->IgnoreParens();
+  Right = Right->IgnoreParens();
+
+  // Compare classes.
+  if (Left->getStmtClass() != Right->getStmtClass())
+    return false;
+
+  // Compare children.
+  Expr::const_child_iterator LeftIter = Left->child_begin();
+  Expr::const_child_iterator RightIter = Right->child_begin();
+  while (LeftIter != Left->child_end() && RightIter != Right->child_end()) {
+    if (!AreIdenticalExpr(dyn_cast<Expr>(*LeftIter),
+                          dyn_cast<Expr>(*RightIter)))
+      return false;
+    ++LeftIter;
+    ++RightIter;
+  }
+  if (LeftIter != Left->child_end() || RightIter != Right->child_end())
+    return false;
+
+  // Perform extra checks.
+  switch (Left->getStmtClass()) {
+  default:
+    return false;
+
+  case Stmt::CharacterLiteralClass:
+    return cast<CharacterLiteral>(Left)->getValue() ==
+           cast<CharacterLiteral>(Right)->getValue();
+  case Stmt::IntegerLiteralClass: {
+    llvm::APInt LeftLit = cast<IntegerLiteral>(Left)->getValue();
+    llvm::APInt RightLit = cast<IntegerLiteral>(Right)->getValue();
+    return LeftLit.getBitWidth() == RightLit.getBitWidth() && LeftLit == RightLit;
+  }
+  case Stmt::FloatingLiteralClass:
+    return cast<FloatingLiteral>(Left)->getValue().bitwiseIsEqual(
+        cast<FloatingLiteral>(Right)->getValue());
+  case Stmt::StringLiteralClass:
+    return cast<StringLiteral>(Left)->getBytes() ==
+           cast<StringLiteral>(Right)->getBytes();
+
+  case Stmt::DeclRefExprClass:
+    return cast<DeclRefExpr>(Left)->getDecl() ==
+           cast<DeclRefExpr>(Right)->getDecl();
+  case Stmt::MemberExprClass:
+    return cast<MemberExpr>(Left)->getMemberDecl() ==
+           cast<MemberExpr>(Right)->getMemberDecl();
+
+  case Stmt::CStyleCastExprClass:
+    return cast<CStyleCastExpr>(Left)->getTypeAsWritten() ==
+           cast<CStyleCastExpr>(Right)->getTypeAsWritten();
+
+  case Stmt::CallExprClass:
+  case Stmt::ImplicitCastExprClass:
+  case Stmt::ArraySubscriptExprClass:
+    return true;
+
+  case Stmt::UnaryOperatorClass:
+    if (cast<UnaryOperator>(Left)->isIncrementDecrementOp())
+      return false;
+    return cast<UnaryOperator>(Left)->getOpcode() ==
+           cast<UnaryOperator>(Right)->getOpcode();
+  case Stmt::BinaryOperatorClass:
+    return cast<BinaryOperator>(Left)->getOpcode() ==
+           cast<BinaryOperator>(Right)->getOpcode();
+  }
+}
+
+AST_MATCHER(BinaryOperator, OperandsAreEquivalent) {
+  return AreIdenticalExpr(Node.getLHS(), Node.getRHS());
+}
+
+AST_MATCHER(BinaryOperator, isInMacro) {
+  return Node.getOperatorLoc().isMacroID();
+}
+
+AST_MATCHER(Expr, isInstantiationDependent) {
+  return Node.isInstantiationDependent();
+}
+
+void RedundantExpressionCheck::registerMatchers(MatchFinder *Finder) {
+  const auto AnyLiteralExpr = ignoringParenImpCasts(
+      anyOf(cxxBoolLiteral(), characterLiteral(), integerLiteral()));
+
+  Finder->addMatcher(
+      binaryOperator(anyOf(hasOperatorName("-"), hasOperatorName("/"),
+                           hasOperatorName("%"), hasOperatorName("|"),
+                           hasOperatorName("&"), hasOperatorName("^"),
+                           matchers::isComparisonOperator(),
+                           hasOperatorName("&&"), hasOperatorName("||"),
+                           hasOperatorName("=")),
+                     OperandsAreEquivalent(),
+                     // Filter noisy false positives.
+                     unless(isInstantiationDependent()),
+                     unless(isInMacro()),
+                     unless(hasType(realFloatingPointType())),
+                     unless(hasEitherOperand(hasType(realFloatingPointType()))),
+                     unless(hasLHS(AnyLiteralExpr)))
+          .bind("binary"),
+      this);
+}
+
+void RedundantExpressionCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *BinOp = Result.Nodes.getNodeAs<BinaryOperator>("binary"))
+    diag(BinOp->getOperatorLoc(), "both side of operator are equivalent");
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.h

@@ -0,0 +1,35 @@
+//===--- RedundantExpressionCheck.h - clang-tidy-----------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_REDUNDANT_EXPRESSION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_REDUNDANT_EXPRESSION_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Detect useless or suspicious redundant expressions.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-redundant-expression.html
+class RedundantExpressionCheck : public ClangTidyCheck {
+public:
+  RedundantExpressionCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_REDUNDANT_EXPRESSION_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -113,6 +113,11 @@ identified.  The improvements since the 3.8 release include:
 
   Warns about suspicious operations involving pointers and integral types.
 
+- New `misc-redundant-expression
+  <http://clang.llvm.org/extra/clang-tidy/checks/misc-redundant-expression.html>`_ check
+
+  Warns about redundant and equivalent expressions.
+
 - New `misc-sizeof-expression
   <http://clang.llvm.org/extra/clang-tidy/checks/misc-sizeof-expression.html>`_ check
 

clang-tools-extra/docs/clang-tidy/checks/list.rst

@@ -67,6 +67,7 @@ Clang-Tidy Checks
    misc-noexcept-move-constructor
    misc-non-copyable-objects
    misc-pointer-and-integral-operation
+   misc-redundant-expression
    misc-sizeof-container
    misc-sizeof-expression
    misc-static-assert

clang-tools-extra/docs/clang-tidy/checks/misc-redundant-expression.rst

@@ -0,0 +1,20 @@
+.. title:: clang-tidy - misc-redundant-expression
+
+misc-redundant-expression
+=========================
+
+Detect redundant expressions which are typically errors due to copy-paste.
+
+Depending on the operator expressions may be
+  * redundant,
+  * always be `true`,
+  * always be `false`,
+  * always be a constant (zero or one)
+
+Example:
+.. code:: c++
+
+  ((x+1) | (x+1))             // (x+1) is redundant
+  (p->x == p->x)              // always true
+  (p->x < p->x)               // always false
+  (speed - speed + 1 == 12)   // speed - speed is always zero

clang-tools-extra/test/clang-tidy/misc-redundant-expression.cpp

@@ -0,0 +1,120 @@
+// RUN: %check_clang_tidy %s misc-redundant-expression %t
+
+struct Point {
+  int x;
+  int y;
+  int a[5];
+} P;
+
+extern Point P1;
+extern Point P2;
+
+extern int foo(int x);
+extern int bar(int x);
+extern int bat(int x, int y);
+
+int Test(int X, int Y) {
+  if (X - X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent [misc-redundant-expression]
+  if (X / X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  if (X % X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+
+  if (X & X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  if (X | X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  if (X ^ X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+
+  if (X < X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  if (X <= X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  if (X > X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  if (X >= X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+
+  if (X && X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  if (X || X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+
+  if (X != (((X)))) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+
+  if (X + 1 == X + 1) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent
+  if (X + 1 != X + 1) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent
+  if (X + 1 <= X + 1) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent
+  if (X + 1 >= X + 1) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent
+
+  if ((X != 1 || Y != 1) && (X != 1 || Y != 1)) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: both side of operator are equivalent
+  if (P.a[X - P.x] != P.a[X - P.x]) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: both side of operator are equivalent
+
+  if ((int)X < (int)X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both side of operator are equivalent
+
+  if ( + "dummy" == + "dummy") return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: both side of operator are equivalent
+  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;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: both side of operator are equivalent  
+  if (foo(bar(0)) < (foo(bar((0))))) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: both side of operator are equivalent  
+
+  if (P1.x < P2.x && P1.x < P2.x) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: both side of operator are equivalent  
+  if (P2.a[P1.x + 2] < P2.x && P2.a[(P1.x) + (2)] < (P2.x)) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: both side of operator are equivalent  
+
+  return 0;
+}
+
+int Valid(int X, int Y) {
+  if (X != Y) return 1;
+  if (X == X + 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;
+
+  if (P.a[0] < P.a[0ULL]) return 1;
+  if (0 < 0ULL) return 1;
+  if ((int)0 < (int)0ULL) return 1;
+
+  if (++X != ++X) return 1;
+  if (P.a[X]++ != P.a[X]++) return 1;
+  if (P.a[X++] != P.a[X++]) return 1;
+
+  if ("abc" == "ABC") return 1;
+  if (foo(bar(0)) < (foo(bat(0, 1)))) return 1;
+  return 0;
+}
+
+#define LT(x, y) (void)((x) < (y))
+
+int TestMacro(int X, int Y) {
+  LT(0, 0);
+  LT(1, 0);
+  LT(X, X);
+  LT(X+1, X + 1);
+}
+
+int TestFalsePositive(int* A, int X, float F) {
+  // Produced by bison.
+  X = A[(2) - (2)];
+  X = A['a' - 'a'];
+
+  // Testing NaN.
+  if (F != F && F == F) return 1;
+  return 0;
+}