https://github.com/SimplyDanny updated https://github.com/llvm/llvm-project/pull/81420
From 4b7d4a46d03c903c6834dfce5b6f379122fdb806 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Danny=20M=C3=B6sch?= <danny.moe...@icloud.com> Date: Sun, 11 Feb 2024 17:04:12 +0100 Subject: [PATCH 1/5] [clang-tidy] Add fix-its to `avoid-return-with-void-value` check --- .../AvoidReturnWithVoidValueCheck.cpp | 41 +++++++++++++++---- clang-tools-extra/docs/ReleaseNotes.rst | 4 ++ .../avoid-return-with-void-value.cpp | 14 ++++++- 3 files changed, 49 insertions(+), 10 deletions(-) diff --git a/clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp b/clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp index e3400f614fa564..1202eeebd06674 100644 --- a/clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp @@ -10,16 +10,18 @@ #include "clang/AST/Stmt.h" #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Basic/Diagnostic.h" +#include "clang/Lex/Lexer.h" using namespace clang::ast_matchers; namespace clang::tidy::readability { -static constexpr auto IgnoreMacrosName = "IgnoreMacros"; -static constexpr auto IgnoreMacrosDefault = true; +static constexpr char IgnoreMacrosName[] = "IgnoreMacros"; +static const bool IgnoreMacrosDefault = true; -static constexpr auto StrictModeName = "StrictMode"; -static constexpr auto StrictModeDefault = true; +static constexpr char StrictModeName[] = "StrictMode"; +static const bool StrictModeDefault = true; AvoidReturnWithVoidValueCheck::AvoidReturnWithVoidValueCheck( StringRef Name, ClangTidyContext *Context) @@ -40,12 +42,35 @@ void AvoidReturnWithVoidValueCheck::registerMatchers(MatchFinder *Finder) { void AvoidReturnWithVoidValueCheck::check( const MatchFinder::MatchResult &Result) { const auto *VoidReturn = Result.Nodes.getNodeAs<ReturnStmt>("void_return"); - if (IgnoreMacros && VoidReturn->getBeginLoc().isMacroID()) + if (IgnoreMacros && VoidReturn->getBeginLoc().isMacroID()) { return; - if (!StrictMode && !Result.Nodes.getNodeAs<CompoundStmt>("compound_parent")) + } + const auto *SurroundingBlock = + Result.Nodes.getNodeAs<CompoundStmt>("compound_parent"); + if (!StrictMode && !SurroundingBlock) { return; - diag(VoidReturn->getBeginLoc(), "return statement within a void function " - "should not have a specified return value"); + } + DiagnosticBuilder Diag = diag(VoidReturn->getBeginLoc(), + "return statement within a void function " + "should not have a specified return value"); + std::optional<Token> SemicolonPos = + Lexer::findNextToken(VoidReturn->getRetValue()->getEndLoc(), + *Result.SourceManager, getLangOpts()); + if (!SemicolonPos) { + return; + } + const StringRef ReturnExpr = + Lexer::getSourceText(CharSourceRange::getTokenRange( + VoidReturn->getRetValue()->getSourceRange()), + *Result.SourceManager, getLangOpts()); + std::string Replacement = (ReturnExpr + "; return;").str(); + if (!SurroundingBlock) { + Replacement = "{" + Replacement + "}"; + } + Diag << FixItHint::CreateReplacement( + CharSourceRange::getTokenRange(VoidReturn->getBeginLoc(), + SemicolonPos->getEndLoc()), + Replacement); } void AvoidReturnWithVoidValueCheck::storeOptions( diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 34bad7e624630c..2babb1406b9776 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -255,6 +255,10 @@ Changes in existing checks analyzed, se the check now handles the common patterns `const auto e = (*vector_ptr)[i]` and `const auto e = vector_ptr->at(i);`. +- Improved :doc:`readability-avoid-return-with-void-value + <clang-tidy/checks/readability/avoid-return-with-void-value>` check by adding + fix-its. + - Improved :doc:`readability-duplicate-include <clang-tidy/checks/readability/duplicate-include>` check by excluding include directives that form the filename using macro. diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/avoid-return-with-void-value.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/avoid-return-with-void-value.cpp index f00407c99ce570..0d269ceee82bc9 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/readability/avoid-return-with-void-value.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/avoid-return-with-void-value.cpp @@ -12,14 +12,18 @@ void f2() { return f1(); // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] // CHECK-MESSAGES-LENIENT: :[[@LINE-2]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] + // CHECK-FIXES: f1(); return; } void f3(bool b) { if (b) return f1(); // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] + // CHECK-FIXES: if (b) {f1(); return;} return f2(); // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] // CHECK-MESSAGES-LENIENT: :[[@LINE-2]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] + // CHECK-FIXES: f2(); return; + // CHECK-FIXES-LENIENT: f2(); return; } template<class T> @@ -29,6 +33,8 @@ void f5() { return f4<void>(); // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] // CHECK-MESSAGES-LENIENT: :[[@LINE-2]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] + // CHECK-FIXES: f4<void>(); return; + // CHECK-FIXES-LENIENT: f4<void>(); return; } void f6() { return; } @@ -41,6 +47,8 @@ void f9() { return (void)f7(); // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] // CHECK-MESSAGES-LENIENT: :[[@LINE-2]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] + // CHECK-FIXES: (void)f7(); return; + // CHECK-FIXES-LENIENT: (void)f7(); return; } #define RETURN_VOID return (void)1 @@ -50,12 +58,12 @@ void f10() { // CHECK-MESSAGES-INCLUDE-MACROS: :[[@LINE-1]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] } -template <typename A> +template <typename A> struct C { C(A) {} }; -template <class T> +template <class T> C<T> f11() { return {}; } using VOID = void; @@ -66,4 +74,6 @@ VOID f13() { return f12(); // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] // CHECK-MESSAGES-LENIENT: :[[@LINE-2]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] + // CHECK-FIXES: f12(); return; + // CHECK-FIXES-LENIENT: f12(); return; } From ebeefa6f4308fd4d4a2e8a78eb734f77a4afd4e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Danny=20M=C3=B6sch?= <danny.moe...@icloud.com> Date: Tue, 20 Feb 2024 22:39:18 +0100 Subject: [PATCH 2/5] Omit blocks for single statements --- .../readability/AvoidReturnWithVoidValueCheck.cpp | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp b/clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp index 1202eeebd06674..f004497e9a3fae 100644 --- a/clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp @@ -42,31 +42,27 @@ void AvoidReturnWithVoidValueCheck::registerMatchers(MatchFinder *Finder) { void AvoidReturnWithVoidValueCheck::check( const MatchFinder::MatchResult &Result) { const auto *VoidReturn = Result.Nodes.getNodeAs<ReturnStmt>("void_return"); - if (IgnoreMacros && VoidReturn->getBeginLoc().isMacroID()) { + if (IgnoreMacros && VoidReturn->getBeginLoc().isMacroID()) return; - } const auto *SurroundingBlock = Result.Nodes.getNodeAs<CompoundStmt>("compound_parent"); - if (!StrictMode && !SurroundingBlock) { + if (!StrictMode && !SurroundingBlock) return; - } DiagnosticBuilder Diag = diag(VoidReturn->getBeginLoc(), "return statement within a void function " "should not have a specified return value"); std::optional<Token> SemicolonPos = Lexer::findNextToken(VoidReturn->getRetValue()->getEndLoc(), *Result.SourceManager, getLangOpts()); - if (!SemicolonPos) { + if (!SemicolonPos) return; - } const StringRef ReturnExpr = Lexer::getSourceText(CharSourceRange::getTokenRange( VoidReturn->getRetValue()->getSourceRange()), *Result.SourceManager, getLangOpts()); std::string Replacement = (ReturnExpr + "; return;").str(); - if (!SurroundingBlock) { + if (!SurroundingBlock) Replacement = "{" + Replacement + "}"; - } Diag << FixItHint::CreateReplacement( CharSourceRange::getTokenRange(VoidReturn->getBeginLoc(), SemicolonPos->getEndLoc()), From 3d36f8192c3e0aeffb27049bec1caec5b0dc464d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Danny=20M=C3=B6sch?= <danny.moe...@icloud.com> Date: Tue, 20 Feb 2024 22:55:40 +0100 Subject: [PATCH 3/5] Use findNextTerminator --- .../readability/AvoidReturnWithVoidValueCheck.cpp | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp b/clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp index f004497e9a3fae..a30f7a3b2670c6 100644 --- a/clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp @@ -7,6 +7,7 @@ //===----------------------------------------------------------------------===// #include "AvoidReturnWithVoidValueCheck.h" +#include "../utils/LexerUtils.h" #include "clang/AST/Stmt.h" #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/ASTMatchers/ASTMatchers.h" @@ -51,10 +52,9 @@ void AvoidReturnWithVoidValueCheck::check( DiagnosticBuilder Diag = diag(VoidReturn->getBeginLoc(), "return statement within a void function " "should not have a specified return value"); - std::optional<Token> SemicolonPos = - Lexer::findNextToken(VoidReturn->getRetValue()->getEndLoc(), - *Result.SourceManager, getLangOpts()); - if (!SemicolonPos) + const SourceLocation SemicolonPos = utils::lexer::findNextTerminator( + VoidReturn->getEndLoc(), *Result.SourceManager, getLangOpts()); + if (SemicolonPos.isInvalid()) return; const StringRef ReturnExpr = Lexer::getSourceText(CharSourceRange::getTokenRange( @@ -64,8 +64,7 @@ void AvoidReturnWithVoidValueCheck::check( if (!SurroundingBlock) Replacement = "{" + Replacement + "}"; Diag << FixItHint::CreateReplacement( - CharSourceRange::getTokenRange(VoidReturn->getBeginLoc(), - SemicolonPos->getEndLoc()), + CharSourceRange::getTokenRange(VoidReturn->getBeginLoc(), SemicolonPos), Replacement); } From 157138942b0f63a89a3e523593d972aeecb7d1c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Danny=20M=C3=B6sch?= <danny.moe...@icloud.com> Date: Fri, 29 Mar 2024 22:02:40 +0100 Subject: [PATCH 4/5] Reuse logic from existing check to insert braces around statements --- .../AvoidReturnWithVoidValueCheck.cpp | 22 +-- .../BracesAroundStatementsCheck.cpp | 157 +++------------- .../readability/BracesAroundStatementsCheck.h | 2 +- .../utils/BracesAroundStatement.cpp | 168 ++++++++++++++++++ .../clang-tidy/utils/BracesAroundStatement.h | 75 ++++++++ .../clang-tidy/utils/CMakeLists.txt | 1 + .../avoid-return-with-void-value.cpp | 30 +++- 7 files changed, 311 insertions(+), 144 deletions(-) create mode 100644 clang-tools-extra/clang-tidy/utils/BracesAroundStatement.cpp create mode 100644 clang-tools-extra/clang-tidy/utils/BracesAroundStatement.h diff --git a/clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp b/clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp index a30f7a3b2670c6..1ce191f342280d 100644 --- a/clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp @@ -7,6 +7,7 @@ //===----------------------------------------------------------------------===// #include "AvoidReturnWithVoidValueCheck.h" +#include "../utils/BracesAroundStatement.h" #include "../utils/LexerUtils.h" #include "clang/AST/Stmt.h" #include "clang/ASTMatchers/ASTMatchFinder.h" @@ -56,16 +57,17 @@ void AvoidReturnWithVoidValueCheck::check( VoidReturn->getEndLoc(), *Result.SourceManager, getLangOpts()); if (SemicolonPos.isInvalid()) return; - const StringRef ReturnExpr = - Lexer::getSourceText(CharSourceRange::getTokenRange( - VoidReturn->getRetValue()->getSourceRange()), - *Result.SourceManager, getLangOpts()); - std::string Replacement = (ReturnExpr + "; return;").str(); - if (!SurroundingBlock) - Replacement = "{" + Replacement + "}"; - Diag << FixItHint::CreateReplacement( - CharSourceRange::getTokenRange(VoidReturn->getBeginLoc(), SemicolonPos), - Replacement); + if (!SurroundingBlock) { + const auto BraceInsertionHints = utils::getBraceInsertionsHints( + VoidReturn, getLangOpts(), *Result.SourceManager, + VoidReturn->getBeginLoc()); + if (BraceInsertionHints) + Diag << BraceInsertionHints.openingBraceFixIt() + << BraceInsertionHints.closingBraceFixIt(); + } + Diag << FixItHint::CreateRemoval(VoidReturn->getReturnLoc()) + << FixItHint::CreateInsertion(SemicolonPos.getLocWithOffset(1), + " return;", true); } void AvoidReturnWithVoidValueCheck::storeOptions( diff --git a/clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp b/clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp index 81ca33cbbdfb4b..85bd9c1e4f9a04 100644 --- a/clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp @@ -7,6 +7,7 @@ //===----------------------------------------------------------------------===// #include "BracesAroundStatementsCheck.h" +#include "../utils/BracesAroundStatement.h" #include "../utils/LexerUtils.h" #include "clang/AST/ASTContext.h" #include "clang/ASTMatchers/ASTMatchers.h" @@ -17,12 +18,10 @@ using namespace clang::ast_matchers; namespace clang::tidy::readability { static tok::TokenKind getTokenKind(SourceLocation Loc, const SourceManager &SM, - const ASTContext *Context) { + const LangOptions &LangOpts) { Token Tok; - SourceLocation Beginning = - Lexer::GetBeginningOfToken(Loc, SM, Context->getLangOpts()); - const bool Invalid = - Lexer::getRawToken(Beginning, Tok, SM, Context->getLangOpts()); + SourceLocation Beginning = Lexer::GetBeginningOfToken(Loc, SM, LangOpts); + const bool Invalid = Lexer::getRawToken(Beginning, Tok, SM, LangOpts); assert(!Invalid && "Expected a valid token."); if (Invalid) @@ -33,64 +32,21 @@ static tok::TokenKind getTokenKind(SourceLocation Loc, const SourceManager &SM, static SourceLocation forwardSkipWhitespaceAndComments(SourceLocation Loc, const SourceManager &SM, - const ASTContext *Context) { + const LangOptions &LangOpts) { assert(Loc.isValid()); for (;;) { while (isWhitespace(*SM.getCharacterData(Loc))) Loc = Loc.getLocWithOffset(1); - tok::TokenKind TokKind = getTokenKind(Loc, SM, Context); + tok::TokenKind TokKind = getTokenKind(Loc, SM, LangOpts); if (TokKind != tok::comment) return Loc; // Fast-forward current token. - Loc = Lexer::getLocForEndOfToken(Loc, 0, SM, Context->getLangOpts()); + Loc = Lexer::getLocForEndOfToken(Loc, 0, SM, LangOpts); } } -static SourceLocation findEndLocation(const Stmt &S, const SourceManager &SM, - const ASTContext *Context) { - SourceLocation Loc = - utils::lexer::getUnifiedEndLoc(S, SM, Context->getLangOpts()); - if (!Loc.isValid()) - return Loc; - - // Start searching right after S. - Loc = Loc.getLocWithOffset(1); - - for (;;) { - assert(Loc.isValid()); - while (isHorizontalWhitespace(*SM.getCharacterData(Loc))) { - Loc = Loc.getLocWithOffset(1); - } - - if (isVerticalWhitespace(*SM.getCharacterData(Loc))) { - // EOL, insert brace before. - break; - } - tok::TokenKind TokKind = getTokenKind(Loc, SM, Context); - if (TokKind != tok::comment) { - // Non-comment token, insert brace before. - break; - } - - SourceLocation TokEndLoc = - Lexer::getLocForEndOfToken(Loc, 0, SM, Context->getLangOpts()); - SourceRange TokRange(Loc, TokEndLoc); - StringRef Comment = Lexer::getSourceText( - CharSourceRange::getTokenRange(TokRange), SM, Context->getLangOpts()); - if (Comment.starts_with("/*") && Comment.contains('\n')) { - // Multi-line block comment, insert brace before. - break; - } - // else: Trailing comment, insert brace after the newline. - - // Fast-forward current token. - Loc = TokEndLoc; - } - return Loc; -} - BracesAroundStatementsCheck::BracesAroundStatementsCheck( StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), @@ -124,7 +80,7 @@ void BracesAroundStatementsCheck::check( } else if (const auto *S = Result.Nodes.getNodeAs<DoStmt>("do")) { checkStmt(Result, S->getBody(), S->getDoLoc(), S->getWhileLoc()); } else if (const auto *S = Result.Nodes.getNodeAs<WhileStmt>("while")) { - SourceLocation StartLoc = findRParenLoc(S, SM, Context); + SourceLocation StartLoc = findRParenLoc(S, SM, Context->getLangOpts()); if (StartLoc.isInvalid()) return; checkStmt(Result, S->getBody(), StartLoc); @@ -133,7 +89,7 @@ void BracesAroundStatementsCheck::check( if (S->isConsteval()) return; - SourceLocation StartLoc = findRParenLoc(S, SM, Context); + SourceLocation StartLoc = findRParenLoc(S, SM, Context->getLangOpts()); if (StartLoc.isInvalid()) return; if (ForceBracesStmts.erase(S)) @@ -156,7 +112,7 @@ template <typename IfOrWhileStmt> SourceLocation BracesAroundStatementsCheck::findRParenLoc(const IfOrWhileStmt *S, const SourceManager &SM, - const ASTContext *Context) { + const LangOptions &LangOpts) { // Skip macros. if (S->getBeginLoc().isMacroID()) return {}; @@ -170,14 +126,14 @@ BracesAroundStatementsCheck::findRParenLoc(const IfOrWhileStmt *S, } SourceLocation PastCondEndLoc = - Lexer::getLocForEndOfToken(CondEndLoc, 0, SM, Context->getLangOpts()); + Lexer::getLocForEndOfToken(CondEndLoc, 0, SM, LangOpts); if (PastCondEndLoc.isInvalid()) return {}; SourceLocation RParenLoc = - forwardSkipWhitespaceAndComments(PastCondEndLoc, SM, Context); + forwardSkipWhitespaceAndComments(PastCondEndLoc, SM, LangOpts); if (RParenLoc.isInvalid()) return {}; - tok::TokenKind TokKind = getTokenKind(RParenLoc, SM, Context); + tok::TokenKind TokKind = getTokenKind(RParenLoc, SM, LangOpts); if (TokKind != tok::r_paren) return {}; return RParenLoc; @@ -188,86 +144,23 @@ 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<AttributedStmt>(S)) S = AS->getSubStmt(); - const SourceManager &SM = *Result.SourceManager; - const ASTContext *Context = Result.Context; - - // 1) If there's a corresponding "else" or "while", the check inserts "} " - // right before that token. - // 2) If there's a multi-line block comment starting on the same line after - // the location we're inserting the closing brace at, or there's a non-comment - // token, the check inserts "\n}" right before that token. - // 3) Otherwise the check finds the end of line (possibly after some block or - // line comments) and inserts "\n}" right before that EOL. - if (!S || isa<CompoundStmt>(S)) { - // Already inside braces. - return false; - } - - // When TreeTransform, Stmt in constexpr IfStmt will be transform to NullStmt. - // This NullStmt can be detected according to beginning token. - const SourceLocation StmtBeginLoc = S->getBeginLoc(); - if (isa<NullStmt>(S) && StmtBeginLoc.isValid() && - getTokenKind(StmtBeginLoc, SM, Context) == tok::l_brace) - return false; - - if (StartLoc.isInvalid()) - return false; - - // 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, - Context->getLangOpts()) - .getBegin(); - if (StartLoc.isInvalid()) - return false; - StartLoc = - Lexer::getLocForEndOfToken(StartLoc, 0, SM, Context->getLangOpts()); - - // StartLoc points at the location of the opening brace to be inserted. - SourceLocation EndLoc; - std::string ClosingInsertion; - if (EndLocHint.isValid()) { - EndLoc = EndLocHint; - ClosingInsertion = "} "; - } else { - EndLoc = findEndLocation(*S, SM, Context); - ClosingInsertion = "\n}"; - } - - assert(StartLoc.isValid()); - - // Don't require braces for statements spanning less than certain number of - // lines. - if (ShortStatementLines && !ForceBracesStmts.erase(S)) { - unsigned StartLine = SM.getSpellingLineNumber(StartLoc); - unsigned EndLine = SM.getSpellingLineNumber(EndLoc); - if (EndLine - StartLine < ShortStatementLines) + const auto BraceInsertionHints = utils::getBraceInsertionsHints( + S, Result.Context->getLangOpts(), *Result.SourceManager, StartLoc, + EndLocHint); + if (BraceInsertionHints) { + if (ShortStatementLines && !ForceBracesStmts.erase(S) && + BraceInsertionHints.resultingCompoundLineExtent(*Result.SourceManager) < + ShortStatementLines) return false; + auto Diag = diag(BraceInsertionHints.DiagnosticPos, + "statement should be inside braces"); + if (BraceInsertionHints.offersFixIts()) + Diag << BraceInsertionHints.openingBraceFixIt() + << BraceInsertionHints.closingBraceFixIt(); } - - auto Diag = diag(StartLoc, "statement should be inside braces"); - - // Change only if StartLoc and EndLoc are on the same macro expansion level. - // This will also catch invalid EndLoc. - // Example: LLVM_DEBUG( for(...) do_something() ); - // In this case fix-it cannot be provided as the semicolon which is not - // visible here is part of the macro. Adding braces here would require adding - // another semicolon. - if (Lexer::makeFileCharRange( - CharSourceRange::getTokenRange(SourceRange( - SM.getSpellingLoc(StartLoc), SM.getSpellingLoc(EndLoc))), - SM, Context->getLangOpts()) - .isInvalid()) - return false; - - Diag << FixItHint::CreateInsertion(StartLoc, " {") - << FixItHint::CreateInsertion(EndLoc, ClosingInsertion); return true; } diff --git a/clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.h b/clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.h index 249aa1aaaa9154..4cd37a7b2dd6cc 100644 --- a/clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.h +++ b/clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.h @@ -52,7 +52,7 @@ class BracesAroundStatementsCheck : public ClangTidyCheck { SourceLocation EndLocHint = SourceLocation()); template <typename IfOrWhileStmt> SourceLocation findRParenLoc(const IfOrWhileStmt *S, const SourceManager &SM, - const ASTContext *Context); + const LangOptions &LangOpts); std::optional<TraversalKind> getCheckTraversalKind() const override { return TK_IgnoreUnlessSpelledInSource; } diff --git a/clang-tools-extra/clang-tidy/utils/BracesAroundStatement.cpp b/clang-tools-extra/clang-tidy/utils/BracesAroundStatement.cpp new file mode 100644 index 00000000000000..2a3b7bed08c1e0 --- /dev/null +++ b/clang-tools-extra/clang-tidy/utils/BracesAroundStatement.cpp @@ -0,0 +1,168 @@ +//===--- BracesAroundStatement.cpp - clang-tidy -------- ------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// +/// +/// \file +/// This file provides utilities to put braces around a statement. +/// +//===----------------------------------------------------------------------===// + +#include "BracesAroundStatement.h" +#include "../utils/LexerUtils.h" +#include "LexerUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/Basic/CharInfo.h" +#include "clang/Basic/LangOptions.h" +#include "clang/Lex/Lexer.h" + +namespace clang::tidy::utils { + +BraceInsertionHints::operator bool() const { return DiagnosticPos.isValid(); } + +bool BraceInsertionHints::offersFixIts() const { + return OpeningBracePos.isValid() && ClosingBracePos.isValid(); +} + +unsigned BraceInsertionHints::resultingCompoundLineExtent( + const SourceManager &SourceMgr) const { + return SourceMgr.getSpellingLineNumber(ClosingBracePos) - + SourceMgr.getSpellingLineNumber(OpeningBracePos); +} + +FixItHint BraceInsertionHints::openingBraceFixIt() const { + return OpeningBracePos.isValid() + ? FixItHint::CreateInsertion(OpeningBracePos, " {") + : FixItHint(); +} + +FixItHint BraceInsertionHints::closingBraceFixIt() const { + return ClosingBracePos.isValid() + ? FixItHint::CreateInsertion(ClosingBracePos, ClosingBrace) + : FixItHint(); +} + +static tok::TokenKind getTokenKind(SourceLocation Loc, const SourceManager &SM, + const LangOptions &LangOpts) { + Token Tok; + SourceLocation Beginning = Lexer::GetBeginningOfToken(Loc, SM, LangOpts); + const bool Invalid = Lexer::getRawToken(Beginning, Tok, SM, LangOpts); + assert(!Invalid && "Expected a valid token."); + + if (Invalid) + return tok::NUM_TOKENS; + + return Tok.getKind(); +} + +static SourceLocation findEndLocation(const Stmt &S, const SourceManager &SM, + const LangOptions &LangOpts) { + SourceLocation Loc = lexer::getUnifiedEndLoc(S, SM, LangOpts); + if (!Loc.isValid()) + return Loc; + + // Start searching right after S. + Loc = Loc.getLocWithOffset(1); + + for (;;) { + assert(Loc.isValid()); + while (isHorizontalWhitespace(*SM.getCharacterData(Loc))) { + Loc = Loc.getLocWithOffset(1); + } + + if (isVerticalWhitespace(*SM.getCharacterData(Loc))) { + // EOL, insert brace before. + break; + } + tok::TokenKind TokKind = getTokenKind(Loc, SM, LangOpts); + if (TokKind != tok::comment) { + // Non-comment token, insert brace before. + break; + } + + SourceLocation TokEndLoc = Lexer::getLocForEndOfToken(Loc, 0, SM, LangOpts); + SourceRange TokRange(Loc, TokEndLoc); + StringRef Comment = Lexer::getSourceText( + CharSourceRange::getTokenRange(TokRange), SM, LangOpts); + if (Comment.starts_with("/*") && Comment.contains('\n')) { + // Multi-line block comment, insert brace before. + break; + } + // else: Trailing comment, insert brace after the newline. + + // Fast-forward current token. + Loc = TokEndLoc; + } + return Loc; +} + +BraceInsertionHints getBraceInsertionsHints(const Stmt *const S, + const LangOptions &LangOpts, + const SourceManager &SM, + SourceLocation StartLoc, + SourceLocation EndLocHint) { + // 1) If there's a corresponding "else" or "while", the check inserts "} " + // right before that token. + // 2) If there's a multi-line block comment starting on the same line after + // the location we're inserting the closing brace at, or there's a non-comment + // token, the check inserts "\n}" right before that token. + // 3) Otherwise the check finds the end of line (possibly after some block or + // line comments) and inserts "\n}" right before that EOL. + if (!S || isa<CompoundStmt>(S)) { + // Already inside braces. + return {}; + } + + // When TreeTransform, Stmt in constexpr IfStmt will be transform to NullStmt. + // This NullStmt can be detected according to beginning token. + const SourceLocation StmtBeginLoc = S->getBeginLoc(); + if (isa<NullStmt>(S) && StmtBeginLoc.isValid() && + getTokenKind(StmtBeginLoc, SM, LangOpts) == tok::l_brace) + return {}; + + if (StartLoc.isInvalid()) + return {}; + + // 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) + .getBegin(); + if (StartLoc.isInvalid()) + return {}; + StartLoc = Lexer::getLocForEndOfToken(StartLoc, 0, SM, LangOpts); + + // StartLoc points at the location of the opening brace to be inserted. + SourceLocation EndLoc; + std::string ClosingInsertion; + if (EndLocHint.isValid()) { + EndLoc = EndLocHint; + ClosingInsertion = "} "; + } else { + EndLoc = findEndLocation(*S, SM, LangOpts); + ClosingInsertion = "\n}"; + } + + assert(StartLoc.isValid()); + + // Change only if StartLoc and EndLoc are on the same macro expansion level. + // This will also catch invalid EndLoc. + // Example: LLVM_DEBUG( for(...) do_something() ); + // In this case fix-it cannot be provided as the semicolon which is not + // visible here is part of the macro. Adding braces here would require adding + // another semicolon. + if (Lexer::makeFileCharRange( + CharSourceRange::getTokenRange(SourceRange( + SM.getSpellingLoc(StartLoc), SM.getSpellingLoc(EndLoc))), + SM, LangOpts) + .isInvalid()) + return {StartLoc}; + return {StartLoc, EndLoc, ClosingInsertion}; +} + +} // namespace clang::tidy::utils diff --git a/clang-tools-extra/clang-tidy/utils/BracesAroundStatement.h b/clang-tools-extra/clang-tidy/utils/BracesAroundStatement.h new file mode 100644 index 00000000000000..cb1c06c7aa1a1a --- /dev/null +++ b/clang-tools-extra/clang-tidy/utils/BracesAroundStatement.h @@ -0,0 +1,75 @@ +//===--- BracesAroundStatement.h - clang-tidy ------- -----------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// +/// +/// \file +/// This file provides utilities to put braces around a statement. +/// +//===----------------------------------------------------------------------===// + +#include "clang/AST/Stmt.h" +#include "clang/Basic/Diagnostic.h" +#include "clang/Basic/SourceLocation.h" +#include "clang/Basic/SourceManager.h" + +namespace clang::tidy::utils { + +/// A provider of fix-it hints to insert opening and closing braces. An instance +/// of this type is the result of calling `getBraceInsertionsHints` below. +struct BraceInsertionHints { + /// The position of a potential diagnostic. It coincides with the position of + /// the opening brace to insert, but can also just be the place to show a + /// diagnostic in case braces cannot be inserted automatically. + SourceLocation DiagnosticPos; + + /// Constructor for a no-hint. + BraceInsertionHints() = default; + + /// Constructor for a valid hint that cannot insert braces automatically. + BraceInsertionHints(SourceLocation DiagnosticPos) + : DiagnosticPos(DiagnosticPos) {} + + /// Constructor for a hint offering fix-its for brace insertion. Both + /// positions must be valid. + BraceInsertionHints(SourceLocation OpeningBracePos, + SourceLocation ClosingBracePos, std::string ClosingBrace) + : DiagnosticPos(OpeningBracePos), OpeningBracePos(OpeningBracePos), + ClosingBracePos(ClosingBracePos), ClosingBrace(ClosingBrace) { + assert(offersFixIts()); + } + + /// Indicates whether the hint provides at least the position of a diagnostic. + operator bool() const; + + /// Indicates whether the hint provides fix-its to insert braces. + bool offersFixIts() const; + + /// The number of lines between the inserted opening brace and its closing + /// counterpart. + unsigned resultingCompoundLineExtent(const SourceManager &SourceMgr) const; + + /// Fix-it to insert an opening brace. + FixItHint openingBraceFixIt() const; + + /// Fix-it to insert a closing brace. + FixItHint closingBraceFixIt() const; + +private: + SourceLocation OpeningBracePos; + SourceLocation ClosingBracePos; + std::string ClosingBrace; +}; + +/// Create fix-it hints for braces that wrap the given statement when applied. +/// The algorithm computing them respects comment before and after the statement +/// and adds line breaks before the braces accordingly. +BraceInsertionHints +getBraceInsertionsHints(const Stmt *const S, const LangOptions &LangOpts, + const SourceManager &SM, SourceLocation StartLoc, + SourceLocation EndLocHint = SourceLocation()); + +} // namespace clang::tidy::utils diff --git a/clang-tools-extra/clang-tidy/utils/CMakeLists.txt b/clang-tools-extra/clang-tidy/utils/CMakeLists.txt index f0160fa9df7487..9cff7d475425d7 100644 --- a/clang-tools-extra/clang-tidy/utils/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/utils/CMakeLists.txt @@ -6,6 +6,7 @@ set(LLVM_LINK_COMPONENTS add_clang_library(clangTidyUtils Aliasing.cpp ASTUtils.cpp + BracesAroundStatement.cpp DeclRefExprUtils.cpp DesignatedInitializers.cpp ExceptionAnalyzer.cpp diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/avoid-return-with-void-value.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/avoid-return-with-void-value.cpp index 0d269ceee82bc9..72571d9e407301 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/readability/avoid-return-with-void-value.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/avoid-return-with-void-value.cpp @@ -18,7 +18,8 @@ void f2() { void f3(bool b) { if (b) return f1(); // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] - // CHECK-FIXES: if (b) {f1(); return;} + // CHECK-FIXES: if (b) { f1(); return; + // CHECK-NEXT: } return f2(); // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] // CHECK-MESSAGES-LENIENT: :[[@LINE-2]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] @@ -77,3 +78,30 @@ VOID f13() { // CHECK-FIXES: f12(); return; // CHECK-FIXES-LENIENT: f12(); return; } + +void f14() { + return /* comment */ f1() /* comment */ ; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] + // CHECK-MESSAGES-LENIENT: :[[@LINE-2]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] + // CHECK-FIXES: /* comment */ f1() /* comment */ ; return; + // CHECK-FIXES-LENIENT: /* comment */ f1() /* comment */ ; return; +} + +void f15() { + return/*comment*/f1()/*comment*/;//comment + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] + // CHECK-MESSAGES-LENIENT: :[[@LINE-2]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] + // CHECK-FIXES: /*comment*/f1()/*comment*/; return;//comment + // CHECK-FIXES-LENIENT: /*comment*/f1()/*comment*/; return;//comment +} + +void f16(bool b) { + if (b) return f1(); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] + // CHECK-FIXES: if (b) { f1(); return; + // CHECK-NEXT: } + else return f2(); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] + // CHECK-FIXES: else { f2(); return; + // CHECK-NEXT: } +} From 91db7ba4ef2ed7fca7f21a671f6f1753416d3ae8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Danny=20M=C3=B6sch?= <danny.moe...@icloud.com> Date: Sat, 30 Mar 2024 19:44:28 +0100 Subject: [PATCH 5/5] Avoid blank return when statement is last in a function body --- .../AvoidReturnWithVoidValueCheck.cpp | 18 +++++++-------- .../avoid-return-with-void-value.cpp | 23 +++++++++++-------- 2 files changed, 22 insertions(+), 19 deletions(-) diff --git a/clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp b/clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp index 1ce191f342280d..48bca41f4a3b1e 100644 --- a/clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp @@ -9,11 +9,6 @@ #include "AvoidReturnWithVoidValueCheck.h" #include "../utils/BracesAroundStatement.h" #include "../utils/LexerUtils.h" -#include "clang/AST/Stmt.h" -#include "clang/ASTMatchers/ASTMatchFinder.h" -#include "clang/ASTMatchers/ASTMatchers.h" -#include "clang/Basic/Diagnostic.h" -#include "clang/Lex/Lexer.h" using namespace clang::ast_matchers; @@ -36,7 +31,10 @@ void AvoidReturnWithVoidValueCheck::registerMatchers(MatchFinder *Finder) { Finder->addMatcher( returnStmt( hasReturnValue(allOf(hasType(voidType()), unless(initListExpr()))), - optionally(hasParent(compoundStmt().bind("compound_parent")))) + optionally(hasParent( + compoundStmt( + optionally(hasParent(functionDecl().bind("function_parent")))) + .bind("compound_parent")))) .bind("void_return"), this); } @@ -65,9 +63,11 @@ void AvoidReturnWithVoidValueCheck::check( Diag << BraceInsertionHints.openingBraceFixIt() << BraceInsertionHints.closingBraceFixIt(); } - Diag << FixItHint::CreateRemoval(VoidReturn->getReturnLoc()) - << FixItHint::CreateInsertion(SemicolonPos.getLocWithOffset(1), - " return;", true); + Diag << FixItHint::CreateRemoval(VoidReturn->getReturnLoc()); + if (!Result.Nodes.getNodeAs<FunctionDecl>("function_parent") || + SurroundingBlock->body_back() != VoidReturn) + Diag << FixItHint::CreateInsertion(SemicolonPos.getLocWithOffset(1), + " return;", true); } void AvoidReturnWithVoidValueCheck::storeOptions( diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/avoid-return-with-void-value.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/avoid-return-with-void-value.cpp index 72571d9e407301..7c948dba3e8f7c 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/readability/avoid-return-with-void-value.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/avoid-return-with-void-value.cpp @@ -12,7 +12,7 @@ void f2() { return f1(); // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] // CHECK-MESSAGES-LENIENT: :[[@LINE-2]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] - // CHECK-FIXES: f1(); return; + // CHECK-FIXES: f1(); } void f3(bool b) { @@ -23,19 +23,19 @@ void f3(bool b) { return f2(); // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] // CHECK-MESSAGES-LENIENT: :[[@LINE-2]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] - // CHECK-FIXES: f2(); return; - // CHECK-FIXES-LENIENT: f2(); return; + // CHECK-FIXES: f2(); + // CHECK-FIXES-LENIENT: f2(); } template<class T> T f4() {} void f5() { - return f4<void>(); - // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] - // CHECK-MESSAGES-LENIENT: :[[@LINE-2]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] - // CHECK-FIXES: f4<void>(); return; - // CHECK-FIXES-LENIENT: f4<void>(); return; + { return f4<void>(); } + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] + // CHECK-MESSAGES-LENIENT: :[[@LINE-2]]:7: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] + // CHECK-FIXES: { f4<void>(); return; } + // CHECK-FIXES-LENIENT: { f4<void>(); return; } } void f6() { return; } @@ -48,8 +48,8 @@ void f9() { return (void)f7(); // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] // CHECK-MESSAGES-LENIENT: :[[@LINE-2]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] - // CHECK-FIXES: (void)f7(); return; - // CHECK-FIXES-LENIENT: (void)f7(); return; + // CHECK-FIXES: (void)f7(); + // CHECK-FIXES-LENIENT: (void)f7(); } #define RETURN_VOID return (void)1 @@ -77,6 +77,7 @@ VOID f13() { // CHECK-MESSAGES-LENIENT: :[[@LINE-2]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] // CHECK-FIXES: f12(); return; // CHECK-FIXES-LENIENT: f12(); return; + (void)1; } void f14() { @@ -85,6 +86,7 @@ void f14() { // CHECK-MESSAGES-LENIENT: :[[@LINE-2]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] // CHECK-FIXES: /* comment */ f1() /* comment */ ; return; // CHECK-FIXES-LENIENT: /* comment */ f1() /* comment */ ; return; + (void)1; } void f15() { @@ -93,6 +95,7 @@ void f15() { // CHECK-MESSAGES-LENIENT: :[[@LINE-2]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value] // CHECK-FIXES: /*comment*/f1()/*comment*/; return;//comment // CHECK-FIXES-LENIENT: /*comment*/f1()/*comment*/; return;//comment + (void)1; } void f16(bool b) { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits