[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,209 @@ 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 Expr *Expr1 = dyn_cast<Expr>(Stmt1);
+  const Expr *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 CStyleCastExpr *CastExpr1 = cast<CStyleCastExpr>(Stmt1);
+    const CStyleCastExpr *CastExpr2 = cast<CStyleCastExpr>(Stmt2);
+
+    return CastExpr1->getTypeAsWritten() == CastExpr2->getTypeAsWritten();
+  }
+  case Stmt::ReturnStmtClass: {
+    const ReturnStmt *ReturnStmt1 = cast<ReturnStmt>(Stmt1);
+    const ReturnStmt *ReturnStmt2 = cast<ReturnStmt>(Stmt2);
+
+    return isIdenticalStmt(Ctx, ReturnStmt1->getRetValue(),
+                           ReturnStmt2->getRetValue(), IgnoreSideEffects);
+  }
+  case Stmt::ForStmtClass: {
+    const ForStmt *ForStmt1 = cast<ForStmt>(Stmt1);
+    const ForStmt *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 DoStmt *DStmt1 = cast<DoStmt>(Stmt1);
+    const DoStmt *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 WhileStmt *WStmt1 = cast<WhileStmt>(Stmt1);
+    const WhileStmt *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 IfStmt *IStmt1 = cast<IfStmt>(Stmt1);
+    const IfStmt *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 CompoundStmt *CompStmt1 = cast<CompoundStmt>(Stmt1);
+    const CompoundStmt *CompStmt2 = cast<CompoundStmt>(Stmt2);
+
+    if (CompStmt1->size() != CompStmt2->size())
+      return false;
+
+    CompoundStmt::const_body_iterator I1 = CompStmt1->body_begin();
+    CompoundStmt::const_body_iterator I2 = CompStmt2->body_begin();
+    while (I1 != CompStmt1->body_end() && I2 != CompStmt2->body_end()) {
+      if (!isIdenticalStmt(Ctx, *I1, *I2, IgnoreSideEffects))
+        return false;
+      ++I1;
+      ++I2;
+    }
+
+    return true;
+  }
+  case Stmt::CompoundAssignOperatorClass:
+  case Stmt::BinaryOperatorClass: {
+    const BinaryOperator *BinOp1 = cast<BinaryOperator>(Stmt1);
+    const BinaryOperator *BinOp2 = cast<BinaryOperator>(Stmt2);
+    return BinOp1->getOpcode() == BinOp2->getOpcode();
+  }
+  case Stmt::CharacterLiteralClass: {
+    const CharacterLiteral *CharLit1 = cast<CharacterLiteral>(Stmt1);
+    const CharacterLiteral *CharLit2 = cast<CharacterLiteral>(Stmt2);
+    return CharLit1->getValue() == CharLit2->getValue();
+  }
+  case Stmt::DeclRefExprClass: {
+    const DeclRefExpr *DeclRef1 = cast<DeclRefExpr>(Stmt1);
+    const DeclRefExpr *DeclRef2 = cast<DeclRefExpr>(Stmt2);
+    return DeclRef1->getDecl() == DeclRef2->getDecl();
+  }
+  case Stmt::IntegerLiteralClass: {
+    const IntegerLiteral *IntLit1 = cast<IntegerLiteral>(Stmt1);
+    const IntegerLiteral *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 FloatingLiteral *FloatLit1 = cast<FloatingLiteral>(Stmt1);
+    const FloatingLiteral *FloatLit2 = cast<FloatingLiteral>(Stmt2);
+    return FloatLit1->getValue().bitwiseIsEqual(FloatLit2->getValue());
+  }
+  case Stmt::StringLiteralClass: {
+    const StringLiteral *StringLit1 = cast<StringLiteral>(Stmt1);
+    const StringLiteral *StringLit2 = cast<StringLiteral>(Stmt2);
+    return StringLit1->getBytes() == StringLit2->getBytes();
+  }
+  case Stmt::MemberExprClass: {
+    const MemberExpr *MemberStmt1 = cast<MemberExpr>(Stmt1);
+    const MemberExpr *MemberStmt2 = cast<MemberExpr>(Stmt2);
+    return MemberStmt1->getMemberDecl() == MemberStmt2->getMemberDecl();
+  }
+  case Stmt::UnaryOperatorClass: {
+    const UnaryOperator *UnaryOp1 = cast<UnaryOperator>(Stmt1);
+    const UnaryOperator *UnaryOp2 = cast<UnaryOperator>(Stmt2);
+    return UnaryOp1->getOpcode() == UnaryOp2->getOpcode();
+  }
+  }
 }
 
 void BranchCloneCheck::check(const MatchFinder::MatchResult &Result) {
@@ -269,6 +472,23 @@ void BranchCloneCheck::check(const MatchFinder::MatchResult &Result) {
     return;
   }
 
+  if (const auto *IS = Result.Nodes.getNodeAs<IfStmt>("ifWithDescendantIf")) {
+    const Stmt *Then = IS->getThen();
+    if (const CompoundStmt *CS = dyn_cast<CompoundStmt>(Then)) {
+      if (!CS->body_empty()) {
+        const IfStmt *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

@@ -865,21 +865,20 @@ 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))
-                   .bind("binary")),
+      traverse(
+          TK_AsIs,
+          binaryOperator(
+              anyOf(isComparisonOperator(),
+                    hasAnyOperatorName("-", "/", "%", "|", "&", "^", "&&", "||",
+                                       "=")),
+              operandsAreEquivalent(),
+              // Filter noisy false positives.
+              unless(isInTemplateInstantiation()),
+              unless(binaryOperatorIsInMacro()),
+              unless(hasAncestor(arraySubscriptExpr())),
+              unless(hasDescendant(BannedIntegerLiteral)),
+              unless(IsInUnevaluatedContext))
+              .bind("binary")),
       this);
 
   // Logical or bitwise operator with equivalent nested operands, like (X && Y
@@ -1238,6 +1237,59 @@ 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();
+    BinaryOperator::Opcode Op = BinOp->getOpcode();
+
+    const DeclRefExpr *DeclRef1 = dyn_cast<DeclRefExpr>(LHS);
+    const DeclRefExpr *DeclRef2 = dyn_cast<DeclRefExpr>(RHS);
+    const FloatingLiteral *FloatLit1 = dyn_cast<FloatingLiteral>(LHS);
+    const FloatingLiteral *FloatLit2 = dyn_cast<FloatingLiteral>(RHS);
+    if ((DeclRef1) && (DeclRef2)) {
+      if ((DeclRef1->getType()->hasFloatingRepresentation()) &&
+          (DeclRef2->getType()->hasFloatingRepresentation())) {
+        if (DeclRef1->getDecl() == DeclRef2->getDecl()) {
+          if ((Op == BO_EQ) || (Op == BO_NE)) {
+            return;
+          }
+        }
+      }
+    } else if ((FloatLit1) && (FloatLit2)) {
+      if (FloatLit1->getValue().bitwiseIsEqual(FloatLit2->getValue())) {
+        if ((Op == BO_EQ) || (Op == BO_NE)) {
+          return;
+        }
+      }
+    } else if (LHS->getType()->hasFloatingRepresentation()) {
+      // If any side of comparison operator still has floating-point
+      // representation, then it's an expression. Don't warn.
+      // Here only LHS is checked since RHS will be implicit casted to float.
+      return;
+    } else {
+      // No special case with floating-point representation, report as usual.
+    }
+
     if (areSidesBinaryConstExpressions(BinOp, Result.Context)) {
       const Expr *LhsConst = nullptr, *RhsConst = nullptr;
       BinaryOperatorKind MainOpcode{}, SideOpcode{};