[analyzer] Port alpha.core.IdenticalExpr to Tidy checks and remove

This change removes the alpha.core.IdenticalExpr static analysis
checker since it's checks are present in the clang-tidy checks
misc-redundant-expression and bugprone-branch-clone. This check was
implemented as a static analysis check using AST matching, and since
alpha and duplicated in 2 clang-tidy checks may be removed. The
existing LIT test was checked case by case, and the tidy checks
were improved to maintain alpha.core.IdenticalExpr features.

Author: Vince Bridgers

clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp

@@ -102,6 +102,208 @@ void BranchCloneCheck::registerMatchers(MatchFinder *Finder) {
       this);
   Finder->addMatcher(switchStmt().bind("switch"), this);
   Finder->addMatcher(conditionalOperator().bind("condOp"), this);
+  Finder->addMatcher(ifStmt(hasDescendant(ifStmt())).bind("ifWithDescendantIf"),
+                     this);
+}
+
+/// Determines whether two statement trees are identical regarding
+/// operators and symbols.
+///
+/// Exceptions: expressions containing macros or functions with possible side
+/// effects are never considered identical.
+/// Limitations: (t + u) and (u + t) are not considered identical.
+/// t*(u + t) and t*u + t*t are not considered identical.
+///
+static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
+                            const Stmt *Stmt2, bool IgnoreSideEffects) {
+
+  if (!Stmt1 || !Stmt2)
+    return !Stmt1 && !Stmt2;
+
+  // If Stmt1 & Stmt2 are of different class then they are not
+  // identical statements.
+  if (Stmt1->getStmtClass() != Stmt2->getStmtClass())
+    return false;
+
+  const auto *Expr1 = dyn_cast<Expr>(Stmt1);
+  const auto *Expr2 = dyn_cast<Expr>(Stmt2);
+
+  if (Expr1 && Expr2) {
+    // If Stmt1 has side effects then don't warn even if expressions
+    // are identical.
+    if (!IgnoreSideEffects && Expr1->HasSideEffects(Ctx))
+      return false;
+    // If either expression comes from a macro then don't warn even if
+    // the expressions are identical.
+    if ((Expr1->getExprLoc().isMacroID()) || (Expr2->getExprLoc().isMacroID()))
+      return false;
+
+    // If all children of two expressions are identical, return true.
+    Expr::const_child_iterator I1 = Expr1->child_begin();
+    Expr::const_child_iterator I2 = Expr2->child_begin();
+    while (I1 != Expr1->child_end() && I2 != Expr2->child_end()) {
+      if (!*I1 || !*I2 || !isIdenticalStmt(Ctx, *I1, *I2, IgnoreSideEffects))
+        return false;
+      ++I1;
+      ++I2;
+    }
+    // If there are different number of children in the statements, return
+    // false.
+    if (I1 != Expr1->child_end())
+      return false;
+    if (I2 != Expr2->child_end())
+      return false;
+  }
+
+  switch (Stmt1->getStmtClass()) {
+  default:
+    return false;
+  case Stmt::CallExprClass:
+  case Stmt::ArraySubscriptExprClass:
+  case Stmt::ArraySectionExprClass:
+  case Stmt::OMPArrayShapingExprClass:
+  case Stmt::OMPIteratorExprClass:
+  case Stmt::ImplicitCastExprClass:
+  case Stmt::ParenExprClass:
+  case Stmt::BreakStmtClass:
+  case Stmt::ContinueStmtClass:
+  case Stmt::NullStmtClass:
+    return true;
+  case Stmt::CStyleCastExprClass: {
+    const auto *CastExpr1 = cast<CStyleCastExpr>(Stmt1);
+    const auto *CastExpr2 = cast<CStyleCastExpr>(Stmt2);
+
+    return CastExpr1->getTypeAsWritten() == CastExpr2->getTypeAsWritten();
+  }
+  case Stmt::ReturnStmtClass: {
+    const auto *ReturnStmt1 = cast<ReturnStmt>(Stmt1);
+    const auto *ReturnStmt2 = cast<ReturnStmt>(Stmt2);
+
+    return isIdenticalStmt(Ctx, ReturnStmt1->getRetValue(),
+                           ReturnStmt2->getRetValue(), IgnoreSideEffects);
+  }
+  case Stmt::ForStmtClass: {
+    const auto *ForStmt1 = cast<ForStmt>(Stmt1);
+    const auto *ForStmt2 = cast<ForStmt>(Stmt2);
+
+    if (!isIdenticalStmt(Ctx, ForStmt1->getInit(), ForStmt2->getInit(),
+                         IgnoreSideEffects))
+      return false;
+    if (!isIdenticalStmt(Ctx, ForStmt1->getCond(), ForStmt2->getCond(),
+                         IgnoreSideEffects))
+      return false;
+    if (!isIdenticalStmt(Ctx, ForStmt1->getInc(), ForStmt2->getInc(),
+                         IgnoreSideEffects))
+      return false;
+    if (!isIdenticalStmt(Ctx, ForStmt1->getBody(), ForStmt2->getBody(),
+                         IgnoreSideEffects))
+      return false;
+    return true;
+  }
+  case Stmt::DoStmtClass: {
+    const auto *DStmt1 = cast<DoStmt>(Stmt1);
+    const auto *DStmt2 = cast<DoStmt>(Stmt2);
+
+    if (!isIdenticalStmt(Ctx, DStmt1->getCond(), DStmt2->getCond(),
+                         IgnoreSideEffects))
+      return false;
+    if (!isIdenticalStmt(Ctx, DStmt1->getBody(), DStmt2->getBody(),
+                         IgnoreSideEffects))
+      return false;
+    return true;
+  }
+  case Stmt::WhileStmtClass: {
+    const auto *WStmt1 = cast<WhileStmt>(Stmt1);
+    const auto *WStmt2 = cast<WhileStmt>(Stmt2);
+
+    if (!isIdenticalStmt(Ctx, WStmt1->getCond(), WStmt2->getCond(),
+                         IgnoreSideEffects))
+      return false;
+    if (!isIdenticalStmt(Ctx, WStmt1->getBody(), WStmt2->getBody(),
+                         IgnoreSideEffects))
+      return false;
+    return true;
+  }
+  case Stmt::IfStmtClass: {
+    const auto *IStmt1 = cast<IfStmt>(Stmt1);
+    const auto *IStmt2 = cast<IfStmt>(Stmt2);
+
+    if (!isIdenticalStmt(Ctx, IStmt1->getCond(), IStmt2->getCond(),
+                         IgnoreSideEffects))
+      return false;
+    if (!isIdenticalStmt(Ctx, IStmt1->getThen(), IStmt2->getThen(),
+                         IgnoreSideEffects))
+      return false;
+    if (!isIdenticalStmt(Ctx, IStmt1->getElse(), IStmt2->getElse(),
+                         IgnoreSideEffects))
+      return false;
+    return true;
+  }
+  case Stmt::CompoundStmtClass: {
+    const auto *CompStmt1 = cast<CompoundStmt>(Stmt1);
+    const auto *CompStmt2 = cast<CompoundStmt>(Stmt2);
+
+    if (CompStmt1->size() != CompStmt2->size())
+      return false;
+
+    if (!llvm::all_of(llvm::zip(CompStmt1->body(), CompStmt2->body()),
+                       [&Ctx, IgnoreSideEffects](std::tuple<const Stmt*, const Stmt*> stmtPair) {
+                          const Stmt *stmt0 = std::get<0>(stmtPair);
+                          const Stmt *stmt1 = std::get<1>(stmtPair);
+                          return isIdenticalStmt(Ctx, stmt0, stmt1, IgnoreSideEffects);
+                       })) {
+        return false;
+    }
+
+    return true;
+  }
+  case Stmt::CompoundAssignOperatorClass:
+  case Stmt::BinaryOperatorClass: {
+    const auto *BinOp1 = cast<BinaryOperator>(Stmt1);
+    const auto *BinOp2 = cast<BinaryOperator>(Stmt2);
+    return BinOp1->getOpcode() == BinOp2->getOpcode();
+  }
+  case Stmt::CharacterLiteralClass: {
+    const auto *CharLit1 = cast<CharacterLiteral>(Stmt1);
+    const auto *CharLit2 = cast<CharacterLiteral>(Stmt2);
+    return CharLit1->getValue() == CharLit2->getValue();
+  }
+  case Stmt::DeclRefExprClass: {
+    const auto *DeclRef1 = cast<DeclRefExpr>(Stmt1);
+    const auto *DeclRef2 = cast<DeclRefExpr>(Stmt2);
+    return DeclRef1->getDecl() == DeclRef2->getDecl();
+  }
+  case Stmt::IntegerLiteralClass: {
+    const auto *IntLit1 = cast<IntegerLiteral>(Stmt1);
+    const auto *IntLit2 = cast<IntegerLiteral>(Stmt2);
+
+    llvm::APInt I1 = IntLit1->getValue();
+    llvm::APInt I2 = IntLit2->getValue();
+    if (I1.getBitWidth() != I2.getBitWidth())
+      return false;
+    return I1 == I2;
+  }
+  case Stmt::FloatingLiteralClass: {
+    const auto *FloatLit1 = cast<FloatingLiteral>(Stmt1);
+    const auto *FloatLit2 = cast<FloatingLiteral>(Stmt2);
+    return FloatLit1->getValue().bitwiseIsEqual(FloatLit2->getValue());
+  }
+  case Stmt::StringLiteralClass: {
+    const auto *StringLit1 = cast<StringLiteral>(Stmt1);
+    const auto *StringLit2 = cast<StringLiteral>(Stmt2);
+    return StringLit1->getBytes() == StringLit2->getBytes();
+  }
+  case Stmt::MemberExprClass: {
+    const auto *MemberStmt1 = cast<MemberExpr>(Stmt1);
+    const auto *MemberStmt2 = cast<MemberExpr>(Stmt2);
+    return MemberStmt1->getMemberDecl() == MemberStmt2->getMemberDecl();
+  }
+  case Stmt::UnaryOperatorClass: {
+    const auto *UnaryOp1 = cast<UnaryOperator>(Stmt1);
+    const auto *UnaryOp2 = cast<UnaryOperator>(Stmt2);
+    return UnaryOp1->getOpcode() == UnaryOp2->getOpcode();
+  }
+  }
 }
 
 void BranchCloneCheck::check(const MatchFinder::MatchResult &Result) {
@@ -269,6 +471,21 @@ void BranchCloneCheck::check(const MatchFinder::MatchResult &Result) {
     return;
   }
 
+  if (const auto *IS = Result.Nodes.getNodeAs<IfStmt>("ifWithDescendantIf")) {
+    const Stmt *Then = IS->getThen();
+    auto CS = dyn_cast<CompoundStmt>(Then);
+    if (CS && (!CS->body_empty())) {
+      const auto *InnerIf = dyn_cast<IfStmt>(*CS->body_begin());
+      if (InnerIf && isIdenticalStmt(Context, IS->getCond(), InnerIf->getCond(),
+                                     /*IgnoreSideEffects=*/false)) {
+        diag(IS->getBeginLoc(), "if with identical inner if statement");
+        diag(InnerIf->getBeginLoc(), "inner if starts here",
+             DiagnosticIDs::Note);
+      }
+    }
+    return;
+  }
+
   llvm_unreachable("No if statement and no switch statement.");
 }
 

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

@@ -855,9 +855,6 @@ static bool areExprsMacroAndNonMacro(const Expr *&LhsExpr,
 } // namespace
 
 void RedundantExpressionCheck::registerMatchers(MatchFinder *Finder) {
-  const auto AnyLiteralExpr = ignoringParenImpCasts(
-      anyOf(cxxBoolLiteral(), characterLiteral(), integerLiteral()));
-
   const auto BannedIntegerLiteral =
       integerLiteral(expandedByMacro(KnownBannedMacroNames));
   const auto IsInUnevaluatedContext = expr(anyOf(
@@ -866,19 +863,16 @@ void RedundantExpressionCheck::registerMatchers(MatchFinder *Finder) {
   // Binary with equivalent operands, like (X != 2 && X != 2).
   Finder->addMatcher(
       traverse(TK_AsIs,
-               binaryOperator(
-                   anyOf(isComparisonOperator(),
-                         hasAnyOperatorName("-", "/", "%", "|", "&", "^", "&&",
-                                            "||", "=")),
-                   operandsAreEquivalent(),
-                   // Filter noisy false positives.
-                   unless(isInTemplateInstantiation()),
-                   unless(binaryOperatorIsInMacro()),
-                   unless(hasType(realFloatingPointType())),
-                   unless(hasEitherOperand(hasType(realFloatingPointType()))),
-                   unless(hasLHS(AnyLiteralExpr)),
-                   unless(hasDescendant(BannedIntegerLiteral)),
-                   unless(IsInUnevaluatedContext))
+               binaryOperator(anyOf(isComparisonOperator(),
+                                    hasAnyOperatorName("-", "/", "%", "|", "&",
+                                                       "^", "&&", "||", "=")),
+                              operandsAreEquivalent(),
+                              // Filter noisy false positives.
+                              unless(isInTemplateInstantiation()),
+                              unless(binaryOperatorIsInMacro()),
+                              unless(hasAncestor(arraySubscriptExpr())),
+                              unless(hasDescendant(BannedIntegerLiteral)),
+                              unless(IsInUnevaluatedContext))
                    .bind("binary")),
       this);
 
@@ -1238,6 +1232,50 @@ void RedundantExpressionCheck::check(const MatchFinder::MatchResult &Result) {
   if (const auto *BinOp = Result.Nodes.getNodeAs<BinaryOperator>("binary")) {
     // If the expression's constants are macros, check whether they are
     // intentional.
+
+    //
+    // Special case for floating-point representation.
+    //
+    // If expressions on both sides of comparison operator are of type float,
+    // then for some comparison operators no warning shall be
+    // reported even if the expressions are identical from a symbolic point of
+    // view. Comparison between expressions, declared variables and literals
+    // are treated differently.
+    //
+    // != and == between float literals that have the same value should NOT
+    // warn. < > between float literals that have the same value SHOULD warn.
+    //
+    // != and == between the same float declaration should NOT warn.
+    // < > between the same float declaration SHOULD warn.
+    //
+    // != and == between eq. expressions that evaluates into float
+    //           should NOT warn.
+    // < >       between eq. expressions that evaluates into float
+    //           should NOT warn.
+    //
+    const Expr *LHS = BinOp->getLHS()->IgnoreParenImpCasts();
+    const Expr *RHS = BinOp->getRHS()->IgnoreParenImpCasts();
+    const BinaryOperator::Opcode Op = BinOp->getOpcode();
+    const bool OpEqualEQorNE = ((Op == BO_EQ) || (Op == BO_NE));
+
+    const auto *DeclRef1 = dyn_cast<DeclRefExpr>(LHS);
+    const auto *DeclRef2 = dyn_cast<DeclRefExpr>(RHS);
+    const auto *FloatLit1 = dyn_cast<FloatingLiteral>(LHS);
+    const auto *FloatLit2 = dyn_cast<FloatingLiteral>(RHS);
+
+    if (DeclRef1 && DeclRef2 &&
+        DeclRef1->getType()->hasFloatingRepresentation() &&
+        DeclRef2->getType()->hasFloatingRepresentation() &&
+        (DeclRef1->getDecl() == DeclRef2->getDecl()) && OpEqualEQorNE) {
+      return;
+    }
+
+    if (FloatLit1 && FloatLit2 &&
+        FloatLit1->getValue().bitwiseIsEqual(FloatLit2->getValue()) &&
+        OpEqualEQorNE) {
+      return;
+    }
+
     if (areSidesBinaryConstExpressions(BinOp, Result.Context)) {
       const Expr *LhsConst = nullptr, *RhsConst = nullptr;
       BinaryOperatorKind MainOpcode{}, SideOpcode{};

clang-tools-extra/docs/ReleaseNotes.rst

@@ -154,6 +154,10 @@ Changes in existing checks
   <clang-tidy/checks/altera/id-dependent-backward-branch>` check by fixing
   crashes from invalid code.
 
+- Improved :doc:`bugprone-branch-clone
+  <clang-tidy/checks/bugprone/branch-clone>` check to improve detection of
+  branch clones by now detecting duplicate inner and outer if statements.
+
 - Improved :doc:`bugprone-casting-through-void
   <clang-tidy/checks/bugprone/casting-through-void>` check to suggest replacing
   the offending code with ``reinterpret_cast``, to more clearly express intent.
@@ -220,6 +224,11 @@ Changes in existing checks
   <clang-tidy/checks/misc/definitions-in-headers>` check by rewording the
   diagnostic note that suggests adding ``inline``.
 
+- Improved :doc:`misc-redundant-expression
+  <clang-tidy/checks/misc/redundant-expression>` check by extending the
+  checker to detect floating point and integer literals in redundant
+  expressions.
+
 - Improved :doc:`misc-unconventional-assign-operator
   <clang-tidy/checks/misc/unconventional-assign-operator>` check to avoid
   false positive for C++23 deducing this.

clang-tools-extra/docs/clang-tidy/checks/bugprone/branch-clone.rst

@@ -32,9 +32,28 @@ If this is the intended behavior, then there is no reason to use a conditional
 statement; otherwise the issue can be solved by fixing the branch that is
 handled incorrectly.
 
-The check also detects repeated branches in longer ``if/else if/else`` chains
+The check detects repeated branches in longer ``if/else if/else`` chains
 where it would be even harder to notice the problem.
 
+The check also detects repeated inner and outer ``if`` statements that may
+be a result of a copy-paste error. This check cannot currently detect
+identical inner and outer ``if`` statements if code is between the ``if``
+conditions. An example is as follows.
+
+.. code-block:: c++
+
+    void test_warn_inner_if_1(int x) {
+      if (x == 1) {    // warns, if with identical inner if
+        if (x == 1)    // inner if is here
+          ;
+      if (x == 1) {    // does not warn, cannot detect
+        int y = x;
+        if (x == 1)
+          ;
+      }
+    }
+
+
 In ``switch`` statements the check only reports repeated branches when they are
 consecutive, because it is relatively common that the ``case:`` labels have
 some natural ordering and rearranging them would decrease the readability of

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

@@ -23,3 +23,22 @@ Examples:
   (p->x == p->x)              // always true
   (p->x < p->x)               // always false
   (speed - speed + 1 == 12)   // speed - speed is always zero
+  int b = a | 4 | a           // identical expr on both sides
+  ((x=1) | (x=1))             // expression is identical
+
+Floats are handled except in the case that NaNs are checked like so:
+
+.. code-block:: c++
+
+  int TestFloat(float F) {
+    if (F == F)               // Identical float values used
+      return 1;
+    return 0;
+  }
+
+  int TestFloat(float F) {
+    // Testing NaN.
+    if (F != F && F == F)     // does not warn
+      return 1;
+    return 0;
+  }