[clang-tidy] Extract areStatementsIdentical

[PiotrZSL]

Summary

Move areStatementsIdentical from BranchCloneCheck into ASTUtils.
Add small improvments. Use it in LoopConvertUtils.

[carlosgalvezp]

Thanks for the refactoring, great to remove some code duplication!

As a reviewer it's not possible for me to confidently review the functional changes to areStatementsIdentical, since they don't show side by side as they are in different files. Please perform the move in one NFC patch, and apply the functional changes in a separate patch.

[PiotrZSL]

Thank you for your feedback regarding the code review. I understand that the review includes the removal of 35 lines and the addition of 42 lines. However, I would like to point out that the areStatementsIdentical function and the other function in question are only around 24 lines long each. Therefore, I believe that the changes made to these functions are not overly complex and can be easily reviewed as they are currently presented.

Additionally, I would like to express my concern that pushing anything through the review process currently takes up to a month. Separating the changes into two patches would only prolong this process and potentially delay the completion of this task.

If you have any questions related to the scope of the changes, I would be more than happy to answer them.

Thank you again for your time and feedback. Please let me know if you have any further concerns, and I'll do my best to address them.

[carlosgalvezp]

Separating the changes into two patches would only prolong this process and potentially delay the completion of this task.

I understand the concern, but in practice it's actually the other way around. 2 small and easy to review patches are more likely to get reviewed quickly than the same patches combined into 1 big patch. The easier you make life for reviewers, the more likely they are to be willing / have time to review a patch. When I post patches, I often try and reflect about what else I can remove or make more obvious/readable to make reviewer's life easier and improve my chances of getting a quick review. I understand it's more work on the author side, but from experience I've found there's a net gain to doing it.

Anyway, the patch is indeed fairly easy to review so I'll just approve it, thank you for the contribution!

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

 //===--- BranchCloneCheck.cpp - clang-tidy --------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "BranchCloneCheck.h"
+#include "../utils/ASTUtils.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Analysis/CloneDetection.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/Support/Casting.h"
 
 using namespace clang;
 using namespace clang::ast_matchers;
 
-/// Returns true when the statements are Type I clones of each other.
-static bool areStatementsIdentical(const Stmt *LHS, const Stmt *RHS,
-                                   const ASTContext &Context) {
-  if (isa<Expr>(LHS) && isa<Expr>(RHS)) {
-    // If we have errors in expressions, we will be unable
-    // to accurately profile and compute hashes for each
-    // of the left and right statements.
-    const auto *LHSExpr = llvm::cast<Expr>(LHS);
-    const auto *RHSExpr = llvm::cast<Expr>(RHS);
-    if (LHSExpr->containsErrors() && RHSExpr->containsErrors()) {
-      return false;
-    }
-  }
-
-  llvm::FoldingSetNodeID DataLHS, DataRHS;
-  LHS->Profile(DataLHS, Context, false);
-  RHS->Profile(DataRHS, Context, false);
-  return (DataLHS == DataRHS);
-}
-
 namespace {
 /// A branch in a switch may consist of several statements; while a branch in
 /// an if/else if/else chain is one statement (which may be a CompoundStmt).
 using SwitchBranch = llvm::SmallVector<const Stmt *, 2>;
 } // anonymous namespace
 
 /// Determines if the bodies of two branches in a switch statements are Type I
 /// clones of each other. This function only examines the body of the branch
 /// and ignores the `case X:` or `default:` at the start of the branch.
 static bool areSwitchBranchesIdentical(const SwitchBranch LHS,
                                        const SwitchBranch RHS,
                                        const ASTContext &Context) {
   if (LHS.size() != RHS.size())
     return false;
 
   for (size_t I = 0, Size = LHS.size(); I < Size; I++) {
     // NOTE: We strip goto labels and annotations in addition to stripping
     // the `case X:` or `default:` labels, but it is very unlikely that this
     // would cause false positives in real-world code.
-    if (!areStatementsIdentical(LHS[I]->stripLabelLikeStatements(),
-                                RHS[I]->stripLabelLikeStatements(), Context)) {
+    if (!tidy::utils::areStatementsIdentical(LHS[I]->stripLabelLikeStatements(),
+                                             RHS[I]->stripLabelLikeStatements(),
+                                             Context)) {
       return false;
     }
   }
 
   return true;
 }
 
 namespace clang::tidy::bugprone {
if (const auto *IS = Result.Nodes.getNodeAs<IfStmt>("if")) {
     const Stmt *Then = IS->getThen();
     assert(Then && "An IfStmt must have a `then` branch!");
 
     const Stmt *Else = Result.Nodes.getNodeAs<Stmt>("else");
     assert(Else && "We only look for `if` statements with an `else` branch!");
 
     if (!isa<IfStmt>(Else)) {
       // Just a simple if with no `else if` branch.
-      if (areStatementsIdentical(Then->IgnoreContainers(),
+      if (utils::areStatementsIdentical(Then->IgnoreContainers(),
                                         Else->IgnoreContainers(), Context)) {
         diag(IS->getBeginLoc(), "if with identical then and else branches");
         diag(IS->getElseLoc(), "else branch starts here", DiagnosticIDs::Note);
       }
       return;
     }
 
     // This is the complicated case when we start an if/else if/else chain.
     // To find all the duplicates, we collect all the branches into a vector.
if (const auto *IS = Result.Nodes.getNodeAs<IfStmt>("if")) {
     for (size_t I = 0; I + 1 < N; I++) {
       // We have already seen Branches[i] as a clone of an earlier branch.
       if (KnownAsClone[I])
         continue;
 
       int NumCopies = 1;
 
       for (size_t J = I + 1; J < N; J++) {
-        if (KnownAsClone[J] ||
-            !areStatementsIdentical(Branches[I]->IgnoreContainers(),
-                                    Branches[J]->IgnoreContainers(), Context))
+        if (KnownAsClone[J] || !utils::areStatementsIdentical(
+                                   Branches[I]->IgnoreContainers(),
           continue;
 
         NumCopies++;
         KnownAsClone[J] = true;
 
         if (NumCopies == 2) {
           // We report the first occurrence only when we find the second one.
           diag(Branches[I]->getBeginLoc(),
for (size_t I = 0; I + 1 < N; I++) {
             << (NumCopies - 1);
       }
     }
     return;
   }
 
   if (const auto *CO = Result.Nodes.getNodeAs<ConditionalOperator>("condOp")) {
     // We do not try to detect chains of ?: operators.
-    if (areStatementsIdentical(CO->getTrueExpr(), CO->getFalseExpr(), Context))
+    if (utils::areStatementsIdentical(CO->getTrueExpr(), CO->getFalseExpr(),
+                                      Context))
       diag(CO->getQuestionLoc(),
            "conditional operator with identical true and false expressions");
 
     return;
   }
 
   if (const auto *SS = Result.Nodes.getNodeAs<SwitchStmt>("switch")) {
     const CompoundStmt *Body = dyn_cast_or_null<CompoundStmt>(SS->getBody());

clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp

 //===--- LoopConvertUtils.cpp - clang-tidy --------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "LoopConvertUtils.h"
+#include "../utils/ASTUtils.h"
 #include "clang/Basic/IdentifierTable.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/Lambda.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TokenKinds.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/APSInt.h"
const Expr *digThroughConstructorsConversions(const Expr *E) {
   if (const auto *ME = dyn_cast<CXXMemberCallExpr>(E))
     if (isa<CXXConversionDecl>(ME->getMethodDecl()))
       return digThroughConstructorsConversions(ME->getImplicitObjectArgument());
   return E;
 }
 
 /// Returns true when two Exprs are equivalent.
 bool areSameExpr(ASTContext *Context, const Expr *First, const Expr *Second) {
-  if (!First || !Second)
-    return false;
-
-  llvm::FoldingSetNodeID FirstID, SecondID;
-  First->Profile(FirstID, *Context, true);
-  Second->Profile(SecondID, *Context, true);
-  return FirstID == SecondID;
+  return utils::areStatementsIdentical(First, Second, *Context, true);
 }
 
 /// Returns the DeclRefExpr represented by E, or NULL if there isn't one.
 const DeclRefExpr *getDeclRef(const Expr *E) {
   return dyn_cast<DeclRefExpr>(E->IgnoreParenImpCasts());
 }
 
 /// Returns true when two ValueDecls are the same variable.

clang-tools-extra/clang-tidy/utils/ASTUtils.h

 
 // Check if the range contains any locations from a macro expansion.
 bool rangeContainsMacroExpansion(SourceRange Range, const SourceManager *SM);
 
 // Can a fix-it be issued for this whole Range?
 // FIXME: false-negative if the entire range is fully expanded from a macro.
 bool rangeCanBeFixed(SourceRange Range, const SourceManager *SM);
 
+// Check if statements are same
+bool areStatementsIdentical(const Stmt *FirstStmt, const Stmt *SecondStmt,
+                            const ASTContext &Context, bool Canonical = false);
+
 } // namespace clang::tidy::utils
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ASTUTILS_H

clang-tools-extra/clang-tidy/utils/ASTUtils.cpp

return rangeIsEntirelyWithinMacroArgument(Range, SM) ||
          Range.getBegin().isMacroID() || Range.getEnd().isMacroID();
 }
 
 bool rangeCanBeFixed(SourceRange Range, const SourceManager *SM) {
   return utils::rangeIsEntirelyWithinMacroArgument(Range, SM) ||
          !utils::rangeContainsMacroExpansion(Range, SM);
 }
 
+bool areStatementsIdentical(const Stmt *FirstStmt, const Stmt *SecondStmt,
+                            const ASTContext &Context, bool Canonical) {
+  if (!FirstStmt || !SecondStmt)
+    return false;
+
+  if (FirstStmt == SecondStmt)
+    return true;
+
+  if (FirstStmt->getStmtClass() != FirstStmt->getStmtClass())
+    return false;
+
+  if (isa<Expr>(FirstStmt) && isa<Expr>(SecondStmt)) {
+    // If we have errors in expressions, we will be unable
+    // to accurately profile and compute hashes for each statements.
+    if (llvm::cast<Expr>(FirstStmt)->containsErrors() ||
+        llvm::cast<Expr>(SecondStmt)->containsErrors())
+      return false;
+  }
+
+  llvm::FoldingSetNodeID DataFirst, DataSecond;
+  FirstStmt->Profile(DataFirst, Context, Canonical);
+  SecondStmt->Profile(DataSecond, Context, Canonical);
+  return DataFirst == DataSecond;
+}
+
 } // namespace clang::tidy::utils