From: Piotr Zegar Date: Tue, 23 May 2023 18:44:55 +0000 (+0000) Subject: [clang-tidy] Improve bugprone-branch-clone with support for fallthrough attribute X-Git-Tag: upstream/17.0.6~7483 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=77f191e81ed475fbbc8eaa2e2dbe6a0a02f3b0c2;p=platform%2Fupstream%2Fllvm.git [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 --- diff --git a/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp index 7657ca7..d27fe0c 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; /// 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 { + using RecursiveASTVisitor::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(A)) + return false; + } + + return true; + } + } Visitor; + + for (const Stmt *Elem : Branch) { + if (!Visitor.TraverseStmt(const_cast(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(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(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 f2393a4..a024bd6 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 + ` check by ignoring duplicated + switch cases marked with the ``[[fallthrough]]`` attribute. + - Improved :doc:`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 9097688..0ca34c2 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 0000000..fb1bebb --- /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; + } +}