From ca1eefdfc061fc062b6b9dc4149ca4a0e51bde08 Mon Sep 17 00:00:00 2001 From: mitchell Date: Sat, 7 Mar 2026 12:17:16 +0800 Subject: [PATCH] [clang-tidy] Correctly handle attributes in readability-inconsistent-ifelse-braces (#184095) 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 --- .../BracesAroundStatementsCheck.cpp | 2 +- .../InconsistentIfElseBracesCheck.cpp | 19 +++++-- .../utils/BracesAroundStatement.cpp | 18 +++++-- .../inconsistent-ifelse-braces-attributes.cpp | 54 +++++++++++++++++-- 4 files changed, 80 insertions(+), 13 deletions(-) diff --git a/clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp b/clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp index 5207c99c10d8..1864dfcbd29c 100644 --- a/clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp @@ -147,7 +147,7 @@ BracesAroundStatementsCheck::findRParenLoc(const IfOrWhileStmt *S, bool BracesAroundStatementsCheck::checkStmt( const MatchFinder::MatchResult &Result, const Stmt *S, SourceLocation StartLoc, SourceLocation EndLocHint) { - while (const auto *AS = dyn_cast(S)) + if (const auto *AS = dyn_cast(S)) S = AS->getSubStmt(); const auto BraceInsertionHints = utils::getBraceInsertionsHints( diff --git a/clang-tools-extra/clang-tidy/readability/InconsistentIfElseBracesCheck.cpp b/clang-tools-extra/clang-tidy/readability/InconsistentIfElseBracesCheck.cpp index 6cc1b203470f..fc22e36a67c5 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(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(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(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(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,10 +70,11 @@ void InconsistentIfElseBracesCheck::checkIfStmt( if (shouldHaveBraces(NestedIf)) checkIfStmt(Result, NestedIf); } else if (!isa(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 Stmt *Else = If->getElse()) { + Else = ignoreAttributed(Else); if (const auto *NestedIf = dyn_cast(Else)) checkIfStmt(Result, NestedIf); else if (!isa(Else)) diff --git a/clang-tools-extra/clang-tidy/utils/BracesAroundStatement.cpp b/clang-tools-extra/clang-tidy/utils/BracesAroundStatement.cpp index 5f804793044e..d51bfd9362eb 100644 --- a/clang-tools-extra/clang-tidy/utils/BracesAroundStatement.cpp +++ b/clang-tools-extra/clang-tidy/utils/BracesAroundStatement.cpp @@ -127,12 +127,24 @@ BraceInsertionHints getBraceInsertionsHints(const Stmt *const S, if (StartLoc.isInvalid()) return {}; + const Stmt *InnerS = S; + while (const auto *AS = dyn_cast(InnerS)) + InnerS = AS->getSubStmt(); + + SourceLocation InsertLoc = StartLoc; + if (S != InnerS) { + if (std::optional Tok = utils::lexer::getPreviousToken( + InnerS->getBeginLoc(), SM, LangOpts, /*SkipComments=*/true)) { + InsertLoc = Tok->getLocation(); + } + } + // Convert StartLoc to file location, if it's on the same macro expansion // level as the start of the statement. We also need file locations for // Lexer::getLocForEndOfToken working properly. - StartLoc = Lexer::makeFileCharRange( - CharSourceRange::getCharRange(StartLoc, S->getBeginLoc()), SM, - LangOpts) + StartLoc = Lexer::makeFileCharRange(CharSourceRange::getCharRange( + InsertLoc, InnerS->getBeginLoc()), + SM, LangOpts) .getBegin(); if (StartLoc.isInvalid()) return {}; 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 8fdb57422702..01965116c327 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 @@ -5,15 +5,29 @@ void f(bool b) { if (b) [[likely]] return; else { } - // CHECK-MESSAGES: :[[@LINE-3]]:9: warning: statement should have braces [readability-inconsistent-ifelse-braces] - // CHECK-FIXES: if (b) { {{[[][[]}}likely{{[]][]]}} return; + // CHECK-MESSAGES: :[[@LINE-3]]:20: warning: statement should have braces [readability-inconsistent-ifelse-braces] + // CHECK-FIXES: if (b) {{[[][[]}}likely{{[]][]]}} { return; // CHECK-FIXES: } else { if (b) { } else [[unlikely]] return; - // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: statement should have braces [readability-inconsistent-ifelse-braces] - // CHECK-FIXES: } else { {{[[][[]}}unlikely{{[]][]]}} + // CHECK-MESSAGES: :[[@LINE-2]]:22: warning: statement should have braces [readability-inconsistent-ifelse-braces] + // CHECK-FIXES: } else {{[[][[]}}unlikely{{[]][]]}} { + + if (b) [[likely]] { + } else [[unlikely]] + return; + // CHECK-MESSAGES: :[[@LINE-2]]:22: warning: statement should have braces [readability-inconsistent-ifelse-braces] + // CHECK-FIXES: } else {{[[][[]}}unlikely{{[]][]]}} { + + if (b) [[likely]] + return; + else [[unlikely]] { + } + // CHECK-MESSAGES: :[[@LINE-4]]:20: 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; + } }