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

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy marked 3 inline comments as done.
donat.nagy added a comment.

**AstDiff:**

I looked at the AstDiff library, but I think that it is overkill for my goals. The main feat of the AstDiff library is analyzing and presenting the differences between two AST subtrees, while this checker only needs a simple yes/no answer for "Are these branches identical?".

**Macros:**

The current implementation of the check only looks at the preprocessed code, therefore it detects the situations where the duplication is created by macros. I added some tests to highlight this behavior. I think that in some situations this would be useful for detecting redundant or incorrect parts of macro-infected 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
----------------
JonasToth wrote:
> maybe plural for the typename would fit better, as its a vector of multiple elements?
This type represents one branch in a switch statement (which consists of multiple statements). I cannot think of a descriptive, short name which also happens to be plural and I do not want to use a monstrosity like `StatementsInSwitchBranch`.

By the way, can anyone think of shorter, but still clear names for `areStatementsIdentical()` and `areSwitchBranchesIdentical()` ?


================
Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:187
+    while (FamilyBegin < End) {
+      auto FamilyEnd = FamilyBegin + 1;
+      while (FamilyEnd < End &&
----------------
JonasToth wrote:
> i think the name `FamilyEnd` is not very descriptive, maybe `SubsequentBranch` or so?
I have renamed FamilyBegin/FamilyEnd to BeginCurrent/EndCurrent. I kept `begin` and `end` in the names to emphasize that this is is an iterator range (and altered the comment to include the expression "iterator range"). Is this naming choice clear enough?


================
Comment at: test/clang-tidy/bugprone-branch-clone.cpp:200
+
+void test_macro1(int in, int &out) {
+  PASTE_CODE(
----------------
The tests for macro handling start here.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54757/new/

https://reviews.llvm.org/D54757