JonasToth added a comment.
Thank you for this great patch!
Did you consider using `clang/Tooling/ASTDiff/*` functionality, as there is the
possibility of just comparing the AST 'below' the branches?
Please add tests that contain macros as well, and what do you think should
happen with them? I think it would be very valuable to figure out that
different macro expansion result in the same code.
================
Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:31
+/// an if/else if/else chain is one statement (which may be a CompoundStmt).
+using SwitchBranch = llvm::SmallVector<const Stmt *, 2>;
+} // anonymous namespace
----------------
maybe plural for the typename would fit better, as its a vector of multiple
elements?
================
Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:46
+ RHS[i]->stripLabelLikeStatements(), Context)) {
+ // NOTE: We strip goto labels and annotations in addition to stripping
+ // the `case X:` or `default:` labels, but it is very unlikely that this
----------------
You can ellide the single-stmt braces here, maybe its better to move the
comment above the for then?
================
Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:71
+ // Check whether this `if` is part of an `else if`:
+ if (const auto *IP =
+ dyn_cast<IfStmt>(Result.Nodes.getNodeAs<Stmt>("ifParent"))) {
----------------
This if should always be true, no? The matcher will always bind `ifParent`
================
Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:83
+ assert(Then && "An IfStmt must have a `then` branch!");
+ const Stmt *Else = IS->getElse();
+ if (!Else) {
----------------
This detection can and should land in the matcher, to filter as much as
possible already while matching.
================
Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:88
+ return;
+ } else if (!isa<IfStmt>(Else)) {
+ // Just a simple if with no `else if` branch.
----------------
Please don't use `else` after return. You can ellide the braces in the `if` as
well.
================
Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:113
+ Cur = dyn_cast<IfStmt>(Else);
+ if (!Cur) {
+ // ...this is just a plain `else` branch; we store it.
----------------
you can ellide the braces.
================
Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:116
+ Branches.push_back(Else);
+ // Note: We will exit the while loop here.
+ }
----------------
This note is misplaced? The exit is at the `break`.
================
Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:120
+
+ const size_t N = Branches.size();
+ llvm::BitVector KnownAsClone(N);
----------------
please dont use `const` for values, only pointers and references
================
Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:124
+ for (size_t i = 0; i + 1 < N; i++) {
+ if (KnownAsClone[i]) {
+ // We have already seen Branches[i] as a clone of an earlier branch.
----------------
braces, i will stop pointing at it. Please do it on the other parts as well
================
Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:153
+ }
+ }
+ } else if (const auto *SS = Result.Nodes.getNodeAs<SwitchStmt>("switch")) {
----------------
i think you can use early return here, if yes, no `else` afterwards.
================
Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:164
+
+ // We will first collect the branhces of the switch statements. For the
+ // sake of simplicity we say that branches are delimited by the SwitchCase
----------------
typo, branhces
================
Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:187
+ while (FamilyBegin < End) {
+ auto FamilyEnd = FamilyBegin + 1;
+ while (FamilyEnd < End &&
----------------
i think the name `FamilyEnd` is not very descriptive, maybe `SubsequentBranch`
or so?
================
Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:196-197
+ diag(FamilyBegin->front()->getBeginLoc(),
+ "switch has %0 consecutive identical branches")
+ << static_cast<int>(FamilyEnd - FamilyBegin);
+ diag(Lexer::getLocForEndOfToken((FamilyEnd - 1)->back()->getEndLoc(),
0,
----------------
please use `std::distance` instead
================
Comment at: docs/clang-tidy/checks/list.rst:13
abseil-str-cat-append
+ abseil-string-find-startswith
android-cloexec-accept
----------------
spurious change
================
Comment at: test/clang-tidy/bugprone-branch-clone.cpp:4
+void test_basic1(int in, int &out) {
+ if (in > 77)
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else
branches [bugprone-branch-clone]
----------------
could you please add tests for ternary operators?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54757
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
