llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy Author: mitchell (zeyi2) <details> <summary>Changes</summary> Improved the check to correctly handle `[[likely]]` and `[[unlikely]]` attributes placed between the if/else keyword and the opening brace. As of AI Usage: Gemini 3 is used for pre-commit reviewing. Closes https://github.com/llvm/llvm-project/issues/184081 --- Full diff: https://github.com/llvm/llvm-project/pull/184095.diff 3 Files Affected: - (modified) clang-tools-extra/clang-tidy/readability/InconsistentIfElseBracesCheck.cpp (+15-6) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+5) - (modified) clang-tools-extra/test/clang-tidy/checkers/readability/inconsistent-ifelse-braces-attributes.cpp (+46) ``````````diff diff --git a/clang-tools-extra/clang-tidy/readability/InconsistentIfElseBracesCheck.cpp b/clang-tools-extra/clang-tidy/readability/InconsistentIfElseBracesCheck.cpp index 6cc1b203470f1..5cf52088917a9 100644 --- a/clang-tools-extra/clang-tidy/readability/InconsistentIfElseBracesCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/InconsistentIfElseBracesCheck.cpp @@ -18,13 +18,21 @@ using namespace clang::ast_matchers; namespace clang::tidy::readability { +/// Look through AttributedStmt wrappers to find the underlying statement. +static const Stmt *ignoreAttributed(const Stmt *S) { + if (const auto *AS = dyn_cast<AttributedStmt>(S)) + return AS->getSubStmt(); + return S; +} + /// Check that at least one branch of the \p If statement is a \c CompoundStmt. static bool shouldHaveBraces(const IfStmt *If) { - const Stmt *const Then = If->getThen(); + const Stmt *const Then = ignoreAttributed(If->getThen()); if (isa<CompoundStmt>(Then)) return true; - if (const Stmt *const Else = If->getElse()) { + if (const Stmt *Else = If->getElse()) { + Else = ignoreAttributed(Else); if (const auto *NestedIf = dyn_cast<const IfStmt>(Else)) return shouldHaveBraces(NestedIf); @@ -53,7 +61,7 @@ void InconsistentIfElseBracesCheck::check( void InconsistentIfElseBracesCheck::checkIfStmt( const MatchFinder::MatchResult &Result, const IfStmt *If) { - const Stmt *Then = If->getThen(); + const Stmt *Then = ignoreAttributed(If->getThen()); if (const auto *NestedIf = dyn_cast<const IfStmt>(Then)) { // If the then-branch is a nested IfStmt, first we need to add braces to // it, then we need to check the inner IfStmt. @@ -62,13 +70,14 @@ void InconsistentIfElseBracesCheck::checkIfStmt( if (shouldHaveBraces(NestedIf)) checkIfStmt(Result, NestedIf); } else if (!isa<CompoundStmt>(Then)) { - emitDiagnostic(Result, Then, If->getRParenLoc(), If->getElseLoc()); + emitDiagnostic(Result, If->getThen(), If->getRParenLoc(), If->getElseLoc()); } if (const Stmt *const Else = If->getElse()) { - if (const auto *NestedIf = dyn_cast<const IfStmt>(Else)) + const Stmt *UnwrappedElse = ignoreAttributed(Else); + if (const auto *NestedIf = dyn_cast<const IfStmt>(UnwrappedElse)) checkIfStmt(Result, NestedIf); - else if (!isa<CompoundStmt>(Else)) + else if (!isa<CompoundStmt>(UnwrappedElse)) emitDiagnostic(Result, If->getElse(), If->getElseLoc()); } } diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index cf8dd0dba9f12..65a24f1311754 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -253,6 +253,11 @@ Changes in existing checks now uses separate note diagnostics for each uninitialized enumerator, making it easier to see which specific enumerators need explicit initialization. +- Improved :doc:`readability-inconsistent-ifelse-braces + <clang-tidy/checks/readability/inconsistent-ifelse-braces>` check by fixing + false positives when ``[[likely]]`` or ``[[unlikely]]`` attributes are placed + between the ``if`` or ``else`` keyword and the opening brace. + - Improved :doc:`readability-non-const-parameter <clang-tidy/checks/readability/non-const-parameter>` check by avoiding false positives on parameters used in dependent expressions (e.g. inside generic diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/inconsistent-ifelse-braces-attributes.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/inconsistent-ifelse-braces-attributes.cpp index 8fdb574227028..8e44440669d40 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/readability/inconsistent-ifelse-braces-attributes.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/inconsistent-ifelse-braces-attributes.cpp @@ -14,6 +14,20 @@ void f(bool b) { return; // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: statement should have braces [readability-inconsistent-ifelse-braces] // CHECK-FIXES: } else { {{[[][[]}}unlikely{{[]][]]}} + + if (b) [[likely]] { + } else [[unlikely]] + return; + // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: statement should have braces [readability-inconsistent-ifelse-braces] + // CHECK-FIXES: } else { {{[[][[]}}unlikely{{[]][]]}} + + if (b) [[likely]] + return; + else [[unlikely]] { + } + // CHECK-MESSAGES: :[[@LINE-4]]:9: warning: statement should have braces [readability-inconsistent-ifelse-braces] + // CHECK-FIXES: if (b) { {{[[][[]}}likely{{[]][]]}} + // CHECK-FIXES: } else {{[[][[]}}unlikely{{[]][]]}} { } // Negative tests. @@ -39,4 +53,36 @@ void g(bool b) { return; else [[likely]] return; + + if (b) [[likely]] { + return; + } else { + return; + } + + if (b) { + return; + } else [[unlikely]] { + return; + } + + if (b) [[likely]] { + return; + } else [[unlikely]] { + return; + } + + if (b) [[likely]] { + return; + } else if (b) [[unlikely]] { + return; + } else { + return; + } + + if (b) [[likely]] [[likely]] { + return; + } else [[unlikely]] [[unlikely]] { + return; + } } `````````` </details> https://github.com/llvm/llvm-project/pull/184095 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
