[clang-tidy] Improve misc-redundant-expression and decrease false-positive

[etienneb]

Summary

This patch is adding support for conditional expression and overloaded operators.

To decrease false-positive, this patch is adding a list of banned macro names that
has multiple variant with same integer value.

Also fixed support for template instantiation and added an unittest.

[etienneb]

nit

[etienneb]

adding unittests

[alexfh]

clang-tidy/misc/RedundantExpressionCheck.cpp

27

Now that you have pulled an API for this, I'll wait until you update this patch.

[etienneb]

use new API

[etienneb]

fix bad upload

[alexfh]

LG.

clang-tidy/misc/RedundantExpressionCheck.cpp

151

Is there a way to not create a set on each call to the matcher? There also might be a more suitable container in llvm/ADT.

200

We should add a hasAnyOverloadedOperatorName matcher at some point.

[etienneb]

We should add a hasAnyOverloadedOperatorName matcher at some point.

I proposed it to sbenza@ and he told me that the reason he did HasAnyName was for speed issue.
I can't tell if that one has speed issue too.

But, otherwise, I agree... for code readability... we should do it.

[etienneb]

remove useless set creation

[etienneb]

fix naming nits

[etienneb]

+ Removed copy of a vector to a set
+ Fixed some minor coding-style issues

clang-tidy/misc/RedundantExpressionCheck.cpp

151

Some ideas:

1. We may assume the vector is small and do the lookup.
2. We may assume a sorted vector and do a binary search.
3. The matcher could receive a set instead of a vector and lifting out the copy.

Implemented 3)

[alexfh]

clang-tidy/misc/RedundantExpressionCheck.cpp

151

Ah, found it: llvm/ADT/StringSet.h seems to be a better choice here.

clang-tidy/misc/RedundantExpressionCheck.cpp

 //===--- 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"
+#include "clang/Lex/Lexer.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
-static bool AreIdenticalExpr(const Expr *Left, const Expr *Right) {
+static const char KnownBannedMacroNames[] =
+    "EAGAIN;EWOULDBLOCK;SIGCLD;SIGCHLD;";
+
+static const char MacroNamesDelimiter[] = ";";
+
+static void ParseMacroNames(StringRef Option,
+                            std::vector<std::string> *Result) {
+  SmallVector<StringRef, 4> Names;
+  Option.split(Names, MacroNamesDelimiter);
+  for (StringRef &Name : Names) {
+    Name = Name.trim();
+    if (!Name.empty())
+      Result->push_back(Name);
+  }
+}
+
+static bool AreEquivalentNameSpecifier(const NestedNameSpecifier *Left,
+                                       const NestedNameSpecifier *Right) {
+  llvm::FoldingSetNodeID LeftID, RightID;
+  Left->Profile(LeftID);
+  Right->Profile(RightID);
+  return LeftID == RightID;
+}
+
+static bool AreEquivalentExpr(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),
+    if (!AreEquivalentExpr(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;
+    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::DependentScopeDeclRefExprClass:
+    if (cast<DependentScopeDeclRefExpr>(Left)->getDeclName() !=
+        cast<DependentScopeDeclRefExpr>(Right)->getDeclName())
+      return false;
+    return AreEquivalentNameSpecifier(
+        cast<DependentScopeDeclRefExpr>(Left)->getQualifier(),
+        cast<DependentScopeDeclRefExpr>(Right)->getQualifier());
   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:
case Stmt::UnaryOperatorClass:
     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, operandsAreEquivalent) {
+  return AreEquivalentExpr(Node.getLHS(), Node.getRHS());
+}
+
+AST_MATCHER(ConditionalOperator, expressionsAreEquivalent) {
+  return AreEquivalentExpr(Node.getTrueExpr(), Node.getFalseExpr());
 }
 
-AST_MATCHER(BinaryOperator, isInMacro) {
+AST_MATCHER(CallExpr, parametersAreEquivalent) {
+  return Node.getNumArgs() == 2 &&
+         AreEquivalentExpr(Node.getArg(0), Node.getArg(1));
+}
+
+AST_MATCHER(BinaryOperator, binaryOperatorIsInMacro) {
   return Node.getOperatorLoc().isMacroID();
 }
 
-AST_MATCHER(Expr, isInstantiationDependent) {
-  return Node.isInstantiationDependent();
+AST_MATCHER(ConditionalOperator, conditionalOperatorIsInMacro) {
+  return Node.getQuestionLoc().isMacroID() || Node.getColonLoc().isMacroID();
+}
+
+AST_MATCHER(Expr, isMacro) { return Node.getExprLoc().isMacroID(); }
+
+AST_MATCHER_P(Expr, expandedByMacro, std::vector<std::string>, NameRefs) {
+  const SourceManager &SM = Finder->getASTContext().getSourceManager();
+  const LangOptions &LO = Finder->getASTContext().getLangOpts();
+  std::set<std::string> Names(NameRefs.begin(), NameRefs.end());
+  SourceLocation Loc = Node.getExprLoc();
+  while (Loc.isMacroID()) {
+    std::string MacroName = Lexer::getImmediateMacroName(Loc, SM, LO);
+    if (Names.find(MacroName) != Names.end())
+      return true;
+    Loc = SM.getImmediateMacroCallerLoc(Loc);
+  }
+  return false;
 }
 
 void RedundantExpressionCheck::registerMatchers(MatchFinder *Finder) {
   const auto AnyLiteralExpr = ignoringParenImpCasts(
       anyOf(cxxBoolLiteral(), characterLiteral(), integerLiteral()));
 
+  std::vector<std::string> MacroNames;
+  ParseMacroNames(KnownBannedMacroNames, &MacroNames);
+  const auto BannedIntegerLiteral = integerLiteral(expandedByMacro(MacroNames));
+
   Finder->addMatcher(
       binaryOperator(anyOf(hasOperatorName("-"), hasOperatorName("/"),
                            hasOperatorName("%"), hasOperatorName("|"),
                            hasOperatorName("&"), hasOperatorName("^"),
                            matchers::isComparisonOperator(),
                            hasOperatorName("&&"), hasOperatorName("||"),
                            hasOperatorName("=")),
-                     OperandsAreEquivalent(),
+                     operandsAreEquivalent(),
                      // Filter noisy false positives.
-                     unless(isInstantiationDependent()),
-                     unless(isInMacro()),
+                     unless(isInTemplateInstantiation()),
+                     unless(binaryOperatorIsInMacro()),
                      unless(hasType(realFloatingPointType())),
                      unless(hasEitherOperand(hasType(realFloatingPointType()))),
-                     unless(hasLHS(AnyLiteralExpr)))
+                     unless(hasLHS(AnyLiteralExpr)),
+                     unless(hasDescendant(BannedIntegerLiteral)))
           .bind("binary"),
       this);
+
+  Finder->addMatcher(
+      conditionalOperator(expressionsAreEquivalent(),
+                          // Filter noisy false positives.
+                          unless(conditionalOperatorIsInMacro()),
+                          unless(hasTrueExpression(AnyLiteralExpr)),
+                          unless(isInTemplateInstantiation()))
+          .bind("cond"),
+      this);
+
+  Finder->addMatcher(
+      cxxOperatorCallExpr(
+          anyOf(
+              hasOverloadedOperatorName("-"), hasOverloadedOperatorName("/"),
+              hasOverloadedOperatorName("%"), hasOverloadedOperatorName("|"),
+              hasOverloadedOperatorName("&"), hasOverloadedOperatorName("^"),
+              hasOverloadedOperatorName("=="), hasOverloadedOperatorName("!="),
+              hasOverloadedOperatorName("<"), hasOverloadedOperatorName("<="),
+              hasOverloadedOperatorName(">"), hasOverloadedOperatorName(">="),
+              hasOverloadedOperatorName("&&"), hasOverloadedOperatorName("||"),
+              hasOverloadedOperatorName("=")),
+          parametersAreEquivalent(),
+          // Filter noisy false positives.
+          unless(isMacro()), unless(isInTemplateInstantiation()))
+          .bind("call"),
+      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");
+  if (const auto *CondOp = Result.Nodes.getNodeAs<ConditionalOperator>("cond"))
+    diag(CondOp->getColonLoc(), "'true' and 'false' expression are equivalent");
+  if (const auto *Call = Result.Nodes.getNodeAs<CXXOperatorCallExpr>("call"))
+    diag(Call->getOperatorLoc(),
+         "both side of overloaded operator are equivalent");
 }
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang

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

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

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

int Test(int X, int Y) {
   // 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  
+  // 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  
+  // 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  
+  // 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  
+  // 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;
 }
 
+int TestConditional(int x, int y) {
+  int k = 0;
+  k += (y < 0) ? x : x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 'true' and 'false' expression are equivalent
+  k += (y < 0) ? x + 1 : x + 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 'true' and 'false' expression are equivalent
+  return k;
+}
+
+struct MyStruct {
+  int x;
+} Q;
+bool operator==(const MyStruct& lhs, const MyStruct& rhs) { return lhs.x == rhs.x; }
+bool TestOperator(const MyStruct& S) {
+  if (S == Q) return false;
+  return S == S;
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: both side of overloaded operator are equivalent
+}
+
 #define LT(x, y) (void)((x) < (y))
+#define COND(x, y, z) ((x)?(y):(z))
+#define EQUALS(x, y) (x) == (y)
 
 int TestMacro(int X, int Y) {
   LT(0, 0);
   LT(1, 0);
   LT(X, X);
   LT(X+1, X + 1);
+  COND(X < Y, X, X);
+  EQUALS(Q, Q);
 }
 
 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;
 }
+
+int TestBannedMacros() {
+#define EAGAIN 3
+#define NOT_EAGAIN 3
+  if (EAGAIN == 0 | EAGAIN == 0) return 0;
+  if (NOT_EAGAIN == 0 | NOT_EAGAIN == 0) return 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: both side of operator are equivalent
+}
+
+struct MyClass {
+static const int Value = 42;
+};
+template <typename T, typename U>
+void TemplateCheck() {
+  static_assert(T::Value == U::Value, "should be identical");
+  static_assert(T::Value == T::Value, "should be identical");
+  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: both side of overloaded operator are equivalent
+}
+void TestTemplate() { TemplateCheck<MyClass, MyClass>(); }