[PATCH] D147889: [clang-tidy] Improve bugprone-branch-clone with support for fallthrough attribute

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

donat.nagy added a comment.

Mostly LGTM, I suggested two cosmetic changes.



================
Comment at: clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp:55
+    bool TraverseLambdaExpr(LambdaExpr *, DataRecursionQueue * = nullptr) {
+      // Ignore lambdas
+      return true;
----------------
Bikeshedding: these comments are very useful, but put them after the return statements. The //(meaningful content) / (number of lines)// ratio is already abysmal in visitor classes like this, let's try to avoid what we can.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp:84
+      for (const Attr *A : S->getAttrs()) {
+        if (FallThroughAttr::classof(A))
+          return false;
----------------
If I understand it correctly, `Class::classof(Pointer)` is an implementation detail of the more flexible and slightly magical `isa<Class>(Obj)` utility. The clang-tidy codebase contains lots of `isa<>` and only a single direct reference to `classof()` so I'd suggest using `isa<>` here for the sake of consistency.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147889