-
Notifications
You must be signed in to change notification settings - Fork 15.5k
[clang-tidy][NFC] Refactor bugprone-branch-clone
#171849
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[clang-tidy][NFC] Refactor bugprone-branch-clone
#171849
Conversation
| if ((Expr1->getExprLoc().isMacroID()) || (Expr2->getExprLoc().isMacroID())) | ||
| return false; | ||
|
|
||
| // If all children of two expressions are identical, return true. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment seems suspicious, that's not what the code is doing, it's only returning false if not all of the children are identical, otherwise it's continuing to the big switch statement. I didn't take the time to investigate it thoroughly, so for now I'm just leaving this as is.
|
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: Victor Chernyakin (localspook) ChangesFull diff: https://github.com/llvm/llvm-project/pull/171849.diff 1 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
index 4f33670a8500a..a1b7c7388fa8e 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
@@ -29,21 +29,14 @@ using SwitchBranch = llvm::SmallVector<const Stmt *, 2>;
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++) {
+ return llvm::equal(LHS, RHS, [&](const Stmt *S1, const Stmt *S2) {
// 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 (!tidy::utils::areStatementsIdentical(LHS[I]->stripLabelLikeStatements(),
- RHS[I]->stripLabelLikeStatements(),
- Context)) {
- return false;
- }
- }
-
- return true;
+ return tidy::utils::areStatementsIdentical(S1->stripLabelLikeStatements(),
+ S2->stripLabelLikeStatements(),
+ Context);
+ });
}
static bool isFallthroughSwitchBranch(const SwitchBranch &Branch) {
@@ -137,19 +130,10 @@ static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
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 (!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())
+ if (!llvm::equal(Expr1->children(), Expr2->children(),
+ [&](const Stmt *S1, const Stmt *S2) {
+ return isIdenticalStmt(Ctx, S1, S2, IgnoreSideEffects);
+ }))
return false;
}
@@ -240,22 +224,10 @@ static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
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;
+ return llvm::equal(CompStmt1->body(), CompStmt2->body(),
+ [&](const Stmt *S1, const Stmt *S2) {
+ return isIdenticalStmt(Ctx, S1, S2, IgnoreSideEffects);
+ });
}
case Stmt::CompoundAssignOperatorClass:
case Stmt::BinaryOperatorClass: {
@@ -450,7 +422,7 @@ void BranchCloneCheck::check(const MatchFinder::MatchResult &Result) {
diag(BeginCurrent->front()->getBeginLoc(),
"switch has %0 consecutive identical branches")
- << static_cast<int>(std::distance(BeginCurrent, EndCurrent));
+ << std::distance(BeginCurrent, EndCurrent);
SourceLocation EndLoc = (EndCurrent - 1)->back()->getEndLoc();
// If the case statement is generated from a macro, it's SourceLocation
|
No description provided.