[clang-tools-extra] 77f191e - [clang-tidy] Improve bugprone-branch-clone with support for fallthrough attribute
Piotr Zegar via cfe-commits
cfe-commits at lists.llvm.org
Tue May 23 12:32:50 PDT 2023
Author: Piotr Zegar
Date: 2023-05-23T19:32:36Z
New Revision: 77f191e81ed475fbbc8eaa2e2dbe6a0a02f3b0c2
URL: https://github.com/llvm/llvm-project/commit/77f191e81ed475fbbc8eaa2e2dbe6a0a02f3b0c2
DIFF: https://github.com/llvm/llvm-project/commit/77f191e81ed475fbbc8eaa2e2dbe6a0a02f3b0c2.diff
LOG: [clang-tidy] Improve bugprone-branch-clone with support for fallthrough attribute
Ignore duplicated switch cases with [[fallthrough]] attribute to reduce false positives.
Fixes: #47588
Reviewed By: donat.nagy
Differential Revision: https://reviews.llvm.org/D147889
Added:
clang-tools-extra/test/clang-tidy/checkers/bugprone/branch-clone-fallthrough.cpp
Modified:
clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/bugprone/branch-clone.rst
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
index 7657ca7380b6f..d27fe0c08d72b 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
@@ -9,6 +9,7 @@
#include "BranchCloneCheck.h"
#include "../utils/ASTUtils.h"
#include "clang/AST/ASTContext.h"
+#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Analysis/CloneDetection.h"
#include "clang/Lex/Lexer.h"
@@ -26,8 +27,8 @@ using SwitchBranch = llvm::SmallVector<const Stmt *, 2>;
/// Determines if the bodies of two branches in a switch statements are Type I
/// clones of each other. This function only examines the body of the branch
/// and ignores the `case X:` or `default:` at the start of the branch.
-static bool areSwitchBranchesIdentical(const SwitchBranch LHS,
- const SwitchBranch RHS,
+static bool areSwitchBranchesIdentical(const SwitchBranch &LHS,
+ const SwitchBranch &RHS,
const ASTContext &Context) {
if (LHS.size() != RHS.size())
return false;
@@ -46,6 +47,50 @@ static bool areSwitchBranchesIdentical(const SwitchBranch LHS,
return true;
}
+static bool isFallthroughSwitchBranch(const SwitchBranch &Branch) {
+ struct SwitchCaseVisitor : RecursiveASTVisitor<SwitchCaseVisitor> {
+ using RecursiveASTVisitor<SwitchCaseVisitor>::DataRecursionQueue;
+
+ bool TraverseLambdaExpr(LambdaExpr *, DataRecursionQueue * = nullptr) {
+ return true; // Ignore lambdas
+ }
+
+ bool TraverseDecl(Decl *) {
+ return true; // No need to check declarations
+ }
+
+ bool TraverseSwitchStmt(SwitchStmt *, DataRecursionQueue * = nullptr) {
+ return true; // Ignore sub-switches
+ }
+
+ bool TraverseSwitchCase(SwitchCase *, DataRecursionQueue * = nullptr) {
+ return true; // Ignore cases
+ }
+
+ bool TraverseDefaultStmt(DefaultStmt *, DataRecursionQueue * = nullptr) {
+ return true; // Ignore defaults
+ }
+
+ bool TraverseAttributedStmt(AttributedStmt *S) {
+ if (!S)
+ return true;
+
+ for (const Attr *A : S->getAttrs()) {
+ if (isa<FallThroughAttr>(A))
+ return false;
+ }
+
+ return true;
+ }
+ } Visitor;
+
+ for (const Stmt *Elem : Branch) {
+ if (!Visitor.TraverseStmt(const_cast<Stmt *>(Elem)))
+ return true;
+ }
+ return false;
+}
+
namespace clang::tidy::bugprone {
void BranchCloneCheck::registerMatchers(MatchFinder *Finder) {
@@ -182,6 +227,11 @@ void BranchCloneCheck::check(const MatchFinder::MatchResult &Result) {
auto *End = Branches.end();
auto *BeginCurrent = Branches.begin();
while (BeginCurrent < End) {
+ if (isFallthroughSwitchBranch(*BeginCurrent)) {
+ ++BeginCurrent;
+ continue;
+ }
+
auto *EndCurrent = BeginCurrent + 1;
while (EndCurrent < End &&
areSwitchBranchesIdentical(*BeginCurrent, *EndCurrent, Context)) {
@@ -189,27 +239,30 @@ void BranchCloneCheck::check(const MatchFinder::MatchResult &Result) {
}
// At this point the iterator range {BeginCurrent, EndCurrent} contains a
// complete family of consecutive identical branches.
- if (EndCurrent > BeginCurrent + 1) {
- diag(BeginCurrent->front()->getBeginLoc(),
- "switch has %0 consecutive identical branches")
- << static_cast<int>(std::distance(BeginCurrent, EndCurrent));
-
- SourceLocation EndLoc = (EndCurrent - 1)->back()->getEndLoc();
- // If the case statement is generated from a macro, it's SourceLocation
- // may be invalid, resulting in an assertion failure down the line.
- // While not optimal, try the begin location in this case, it's still
- // better then nothing.
- if (EndLoc.isInvalid())
- EndLoc = (EndCurrent - 1)->back()->getBeginLoc();
-
- if (EndLoc.isMacroID())
- EndLoc = Context.getSourceManager().getExpansionLoc(EndLoc);
- EndLoc = Lexer::getLocForEndOfToken(EndLoc, 0, *Result.SourceManager,
- getLangOpts());
-
- if (EndLoc.isValid()) {
- diag(EndLoc, "last of these clones ends here", DiagnosticIDs::Note);
- }
+
+ if (EndCurrent == (BeginCurrent + 1)) {
+ // No consecutive identical branches that start on BeginCurrent
+ BeginCurrent = EndCurrent;
+ continue;
+ }
+
+ diag(BeginCurrent->front()->getBeginLoc(),
+ "switch has %0 consecutive identical branches")
+ << static_cast<int>(std::distance(BeginCurrent, EndCurrent));
+
+ SourceLocation EndLoc = (EndCurrent - 1)->back()->getEndLoc();
+ // If the case statement is generated from a macro, it's SourceLocation
+ // may be invalid, resulting in an assertion failure down the line.
+ // While not optimal, try the begin location in this case, it's still
+ // better then nothing.
+ if (EndLoc.isInvalid())
+ EndLoc = (EndCurrent - 1)->back()->getBeginLoc();
+ if (EndLoc.isMacroID())
+ EndLoc = Context.getSourceManager().getExpansionLoc(EndLoc);
+ EndLoc = Lexer::getLocForEndOfToken(EndLoc, 0, *Result.SourceManager,
+ getLangOpts());
+ if (EndLoc.isValid()) {
+ diag(EndLoc, "last of these clones ends here", DiagnosticIDs::Note);
}
BeginCurrent = EndCurrent;
}
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index f2393a4156548..a024bd604d16e 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -201,6 +201,11 @@ New check aliases
Changes in existing checks
^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+- Fixed false-positive in :doc:`bugprone-branch-clone
+ <clang-tidy/checks/bugprone/branch-clone>` check by ignoring duplicated
+ switch cases marked with the ``[[fallthrough]]`` attribute.
+
- Improved :doc:`readability-redundant-string-cstr
<clang-tidy/checks/readability/redundant-string-cstr>` check to recognise
unnecessary ``std::string::c_str()`` and ``std::string::data()`` calls in
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/branch-clone.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/branch-clone.rst
index 9097688c46774..0ca34c2bc2320 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/branch-clone.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/branch-clone.rst
@@ -77,6 +77,8 @@ Here the check does not warn for the repeated ``return 10;``, which is good if
we want to preserve that ``'a'`` is before ``'b'`` and ``default:`` is the last
branch.
+Switch cases marked with the ``[[fallthrough]]`` attribute are ignored.
+
Finally, the check also examines conditional operators and reports code like:
.. code-block:: c++
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/branch-clone-fallthrough.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/branch-clone-fallthrough.cpp
new file mode 100644
index 0000000000000..fb1bebb78b6ce
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/branch-clone-fallthrough.cpp
@@ -0,0 +1,65 @@
+// RUN: %check_clang_tidy %s bugprone-branch-clone %t -- -- -std=c++17
+
+void handle(int);
+
+void testSwitchFallthroughAttribute(int value) {
+ switch(value) {
+ case 1: [[fallthrough]];
+ case 2: [[fallthrough]];
+ case 3:
+ handle(value);
+ break;
+ default:
+ break;
+ }
+}
+
+void testSwitchFallthroughAttributeAndBraces(int value) {
+ switch(value) {
+ case 1: { [[fallthrough]]; }
+ case 2: { [[fallthrough]]; }
+ case 3: {
+ handle(value);
+ break;
+ }
+ default: {
+ break;
+ }
+ }
+}
+
+void testSwitchWithFallthroughAttributeAndCode(int value) {
+ switch(value) {
+ case 1: value += 1; [[fallthrough]];
+ case 2: value += 1; [[fallthrough]];
+ case 3:
+ handle(value);
+ break;
+ default:
+ break;
+ }
+}
+
+void testSwitchWithFallthroughAndCode(int value) {
+ switch(value) {
+ // CHECK-MESSAGES: :[[@LINE+1]]:5: warning: switch has 2 consecutive identical branches [bugprone-branch-clone]
+ case 1: value += 1;
+ case 2: value += 1;
+ // CHECK-MESSAGES: :[[@LINE-1]]:23: note: last of these clones ends here
+ case 3:
+ handle(value);
+ break;
+ default:
+ break;
+ }
+}
+
+void testSwitchFallthroughAttributeIntoDefault(int value) {
+ switch(value) {
+ case 1: [[fallthrough]];
+ case 2: [[fallthrough]];
+ default:
+ handle(value);
+ break;
+ }
+}
More information about the cfe-commits
mailing list