[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

[Jonas Toth via Phabricator via cfe-commits]

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