Index: clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp =================================================================== --- clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp +++ clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp @@ -7,6 +7,7 @@ //===----------------------------------------------------------------------===// #include "BracesAroundStatementsCheck.h" +#include "../utils/LexerUtils.h" #include "clang/AST/ASTContext.h" #include "clang/ASTMatchers/ASTMatchers.h" #include "clang/Lex/Lexer.h" @@ -16,10 +17,9 @@ namespace clang { namespace tidy { namespace readability { -namespace { -tok::TokenKind getTokenKind(SourceLocation Loc, const SourceManager &SM, - const ASTContext *Context) { +static tok::TokenKind getTokenKind(SourceLocation Loc, const SourceManager &SM, + const ASTContext *Context) { Token Tok; SourceLocation Beginning = Lexer::GetBeginningOfToken(Loc, SM, Context->getLangOpts()); @@ -33,9 +33,9 @@ return Tok.getKind(); } -SourceLocation forwardSkipWhitespaceAndComments(SourceLocation Loc, - const SourceManager &SM, - const ASTContext *Context) { +static SourceLocation +forwardSkipWhitespaceAndComments(SourceLocation Loc, const SourceManager &SM, + const ASTContext *Context) { assert(Loc.isValid()); for (;;) { while (isWhitespace(*SM.getCharacterData(Loc))) @@ -50,31 +50,15 @@ } } -SourceLocation findEndLocation(SourceLocation LastTokenLoc, - const SourceManager &SM, - const ASTContext *Context) { +static SourceLocation findEndLocation(const Stmt &S, const SourceManager &SM, + const ASTContext *Context) { SourceLocation Loc = - Lexer::GetBeginningOfToken(LastTokenLoc, SM, Context->getLangOpts()); - // Loc points to the beginning of the last (non-comment non-ws) token - // before end or ';'. - assert(Loc.isValid()); - bool SkipEndWhitespaceAndComments = true; - tok::TokenKind TokKind = getTokenKind(Loc, SM, Context); - if (TokKind == tok::NUM_TOKENS || TokKind == tok::semi || - TokKind == tok::r_brace) { - // If we are at ";" or "}", we found the last token. We could use as well - // `if (isa(S))`, but it wouldn't work for nested statements. - SkipEndWhitespaceAndComments = false; - } + utils::lexer::getUnifiedEndLoc(S, SM, Context->getLangOpts()); + if (!Loc.isValid()) + return Loc; - Loc = Lexer::getLocForEndOfToken(Loc, 0, SM, Context->getLangOpts()); - // Loc points past the last token before end or after ';'. - if (SkipEndWhitespaceAndComments) { - Loc = forwardSkipWhitespaceAndComments(Loc, SM, Context); - tok::TokenKind TokKind = getTokenKind(Loc, SM, Context); - if (TokKind == tok::semi) - Loc = Lexer::getLocForEndOfToken(Loc, 0, SM, Context->getLangOpts()); - } + // Start searching right after S. + Loc = Loc.getLocWithOffset(1); for (;;) { assert(Loc.isValid()); @@ -109,8 +93,6 @@ return Loc; } -} // namespace - BracesAroundStatementsCheck::BracesAroundStatementsCheck( StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), @@ -223,13 +205,6 @@ const SourceManager &SM = *Result.SourceManager; const ASTContext *Context = Result.Context; - // Treat macros. - CharSourceRange FileRange = Lexer::makeFileCharRange( - CharSourceRange::getTokenRange(S->getSourceRange()), SM, - Context->getLangOpts()); - if (FileRange.isInvalid()) - return false; - // Convert InitialLoc 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. @@ -249,13 +224,12 @@ EndLoc = EndLocHint; ClosingInsertion = "} "; } else { - const auto FREnd = FileRange.getEnd().getLocWithOffset(-1); - EndLoc = findEndLocation(FREnd, SM, Context); + EndLoc = findEndLocation(*S, SM, Context); ClosingInsertion = "\n}"; } assert(StartLoc.isValid()); - assert(EndLoc.isValid()); + // Don't require braces for statements spanning less than certain number of // lines. if (ShortStatementLines && !ForceBracesStmts.erase(S)) { @@ -266,6 +240,20 @@ } 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; Index: clang-tools-extra/clang-tidy/utils/LexerUtils.h =================================================================== --- clang-tools-extra/clang-tidy/utils/LexerUtils.h +++ clang-tools-extra/clang-tidy/utils/LexerUtils.h @@ -14,6 +14,9 @@ #include "clang/Lex/Lexer.h" namespace clang { + +class Stmt; + namespace tidy { namespace utils { namespace lexer { @@ -104,6 +107,11 @@ const ASTContext &Context, const SourceManager &SM); +/// Stmt->getEndLoc does not always behave the same way depending on Token type. +/// See implementation for exceptions. +SourceLocation getUnifiedEndLoc(const Stmt &S, const SourceManager &SM, + const LangOptions &LangOpts); + } // namespace lexer } // namespace utils } // namespace tidy Index: clang-tools-extra/clang-tidy/utils/LexerUtils.cpp =================================================================== --- clang-tools-extra/clang-tidy/utils/LexerUtils.cpp +++ clang-tools-extra/clang-tidy/utils/LexerUtils.cpp @@ -7,6 +7,7 @@ //===----------------------------------------------------------------------===// #include "LexerUtils.h" +#include "clang/AST/AST.h" #include "clang/Basic/SourceManager.h" namespace clang { @@ -148,6 +149,70 @@ return LastMatchAfterTemplate != None ? LastMatchAfterTemplate : LastMatchBeforeTemplate; } + +static bool breakAndReturnEnd(const Stmt &S) { + return isa(S); +} + +static bool breakAndReturnEndPlus1Token(const Stmt &S) { + return isa(S); +} + +// Given a Stmt which does not include it's semicolon this method returns the +// SourceLocation of the semicolon. +static SourceLocation getSemicolonAfterStmtEndLoc(const SourceLocation &EndLoc, + const SourceManager &SM, + const LangOptions &LangOpts) { + + if (EndLoc.isMacroID()) { + // Assuming EndLoc points to a function call foo within macro F. + // This method is supposed to return location of the semicolon within + // those macro arguments: + // F ( foo() ; ) + // ^ EndLoc ^ SpellingLoc ^ next token of SpellingLoc + const SourceLocation SpellingLoc = SM.getSpellingLoc(EndLoc); + Optional NextTok = + findNextTokenSkippingComments(SpellingLoc, SM, LangOpts); + + // Was the next token found successfully? + // All macro issues are simply resolved by ensuring it's a semicolon. + if (NextTok && NextTok->is(tok::TokenKind::semi)) { + // Ideally this would return `F` with spelling location `;` (NextTok) + // following the examle above. For now simply return NextTok location. + return NextTok->getLocation(); + } + + // Fallthrough to 'normal handling'. + // F ( foo() ) ; + // ^ EndLoc ^ SpellingLoc ) ^ next token of EndLoc + } + + Optional NextTok = findNextTokenSkippingComments(EndLoc, SM, LangOpts); + + // Testing for semicolon again avoids some issues with macros. + if (NextTok && NextTok->is(tok::TokenKind::semi)) + return NextTok->getLocation(); + + return SourceLocation(); +} + +SourceLocation getUnifiedEndLoc(const Stmt &S, const SourceManager &SM, + const LangOptions &LangOpts) { + + const Stmt *LastChild = &S; + while (!LastChild->children().empty() && !breakAndReturnEnd(*LastChild) && + !breakAndReturnEndPlus1Token(*LastChild)) { + for (const Stmt *Child : LastChild->children()) + LastChild = Child; + } + + if (!breakAndReturnEnd(*LastChild) && + breakAndReturnEndPlus1Token(*LastChild)) + return getSemicolonAfterStmtEndLoc(S.getEndLoc(), SM, LangOpts); + + return S.getEndLoc(); +} + } // namespace lexer } // namespace utils } // namespace tidy Index: clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements.cpp =================================================================== --- clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements.cpp +++ clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements.cpp @@ -74,30 +74,41 @@ do_something("for"); // CHECK-MESSAGES: :[[@LINE-2]]:11: warning: statement should be inside braces // CHECK-FIXES: for (;;) { - // CHECK-FIXES: } + // CHECK-FIXES-NEXT: do_something("for"); + // CHECK-FIXES-NEXT: } + for (;;) { - do_something("for"); + do_something("for-ok"); } for (;;) ; // CHECK-MESSAGES: :[[@LINE-2]]:11: warning: statement should be inside braces // CHECK-FIXES: for (;;) { - // CHECK-FIXES: } + // CHECK-FIXES-NEXT: ; + // CHECK-FIXES-NEXT: } int arr[4] = {1, 2, 3, 4}; for (int a : arr) do_something("for-range"); // CHECK-MESSAGES: :[[@LINE-2]]:20: warning: statement should be inside braces // CHECK-FIXES: for (int a : arr) { - // CHECK-FIXES: } - for (int a : arr) { + // CHECK-FIXES-NEXT: do_something("for-range"); + // CHECK-FIXES-NEXT: } + for (int &assign : arr) + assign = 7; + // CHECK-MESSAGES: :[[@LINE-2]]:26: warning: statement should be inside braces + // CHECK-FIXES: for (int &assign : arr) { + // CHECK-FIXES-NEXT: assign = 7; + // CHECK-FIXES-NEXT: } + for (int ok : arr) { do_something("for-range"); } - for (int a : arr) + for (int NullStmt : arr) ; - // CHECK-MESSAGES: :[[@LINE-2]]:20: warning: statement should be inside braces - // CHECK-FIXES: for (int a : arr) { - // CHECK-FIXES: } + // CHECK-MESSAGES: :[[@LINE-2]]:27: warning: statement should be inside braces + // CHECK-FIXES: for (int NullStmt : arr) { + // CHECK-FIXES-NEXT: ; + // CHECK-FIXES-NEXT: } while (cond("while1")) do_something("while"); @@ -143,14 +154,19 @@ // CHECK-FIXES-NEXT: } if (cond("ifif3")) - // comment + // comment1 if (cond("ifif4")) { - // comment - /*comment*/; // comment + // comment2 + /*comment3*/; // comment4 } // CHECK-MESSAGES: :[[@LINE-6]]:21: warning: statement should be inside braces // CHECK-FIXES: if (cond("ifif3")) { - // CHECK-FIXES: } + // CHECK-FIXES-NEXT: // comment1 + // CHECK-FIXES-NEXT: if (cond("ifif4")) { + // CHECK-FIXES-NEXT: // comment2 + // CHECK-FIXES-NEXT: /*comment3*/; // comment4 + // CHECK-FIXES-NEXT: } + // CHECK-FIXES-NEXT: } if (cond("ifif5")) ; /* multi-line @@ -170,6 +186,161 @@ // CHECK-FIXES-NEXT: } // CHECK-FIXES-NEXT: } // CHECK-FIXES-NEXT: } + + int S; + if (cond("assign with brackets")) + S = {5}; + // CHECK-MESSAGES: :[[@LINE-2]]:36: warning: statement should be inside braces + // CHECK-FIXES: if (cond("assign with brackets")) { + // CHECK-FIXES-NEXT: S = {5}; + // CHECK-FIXES-NEXT: } + + if (cond("assign with brackets 2")) + S = { 5 } /* comment1 */ ; /* comment2 */ + // CHECK-MESSAGES: :[[@LINE-2]]:38: warning: statement should be inside braces + // CHECK-FIXES: if (cond("assign with brackets 2")) { + // CHECK-FIXES-NEXT: S = { 5 } /* comment1 */ ; /* comment2 */ + // CHECK-FIXES-NEXT: } + + if (cond("return")) + return; + // CHECK-MESSAGES: :[[@LINE-2]]:22: warning: statement should be inside braces + // CHECK-FIXES: if (cond("return")) { + // CHECK-FIXES-NEXT: return; + // CHECK-FIXES-NEXT: } + + while (cond("break and continue")) { + // CHECK-FIXES: while (cond("break and continue")) { + if (true) + break; + // CHECK-MESSAGES: :[[@LINE-2]]:14: warning: statement should be inside braces + // CHECK-FIXES: {{^}} if (true) {{{$}} + // CHECK-FIXES-NEXT: {{^}} break;{{$}} + // CHECK-FIXES-NEXT: {{^ *}}}{{$}} + if (false) + continue; + // CHECK-MESSAGES: :[[@LINE-2]]:15: warning: statement should be inside braces + // CHECK-FIXES: {{^}} if (false) {{{$}} + // CHECK-FIXES-NEXT: {{^}} continue;{{$}} + // CHECK-FIXES-NEXT: {{^ *}}}{{$}} + } //end + // CHECK-FIXES: } //end + + if (cond("decl 1")) + int s; + else + int t; + // CHECK-MESSAGES: :[[@LINE-4]]:22: warning: statement should be inside braces + // CHECK-MESSAGES: :[[@LINE-3]]:7: warning: statement should be inside braces + // CHECK-FIXES: if (cond("decl 1")) { + // CHECK-FIXES-NEXT: int s; + // CHECK-FIXES-NEXT: } else { + // CHECK-FIXES-NEXT: int t; + // CHECK-FIXES-NEXT: } + + if (cond("decl 2")) + int s = (5); + else + int t = (5); + // CHECK-MESSAGES: :[[@LINE-4]]:22: warning: statement should be inside braces + // CHECK-MESSAGES: :[[@LINE-3]]:7: warning: statement should be inside braces + // CHECK-FIXES: if (cond("decl 2")) { + // CHECK-FIXES-NEXT: int s = (5); + // CHECK-FIXES-NEXT: } else { + // CHECK-FIXES-NEXT: int t = (5); + // CHECK-FIXES-NEXT: } + + if (cond("decl 3")) + int s = {6}; + else + int t = {6}; + // CHECK-MESSAGES: :[[@LINE-4]]:22: warning: statement should be inside braces + // CHECK-MESSAGES: :[[@LINE-3]]:7: warning: statement should be inside braces + // CHECK-FIXES: if (cond("decl 3")) { + // CHECK-FIXES-NEXT: int s = {6}; + // CHECK-FIXES-NEXT: } else { + // CHECK-FIXES-NEXT: int t = {6}; + // CHECK-FIXES-NEXT: } +} + +void test_whitespace() { + while(cond("preserve empty lines")) + if(cond("using continue within if")) + continue; + + + test(); + + // CHECK-MESSAGES: :[[@LINE-7]]:{{[0-9]+}}: warning: statement should be inside braces + // CHECK-MESSAGES: :[[@LINE-7]]:{{[0-9]+}}: warning: statement should be inside braces + // CHECK-FIXES: {{^}} while(cond("preserve empty lines")) {{{$}} + // CHECK-FIXES-NEXT: {{^}} if(cond("using continue within if")) {{{$}} + // CHECK-FIXES-NEXT: {{^ continue;$}} + // The closing brace is added at beginning of line, clang-format can be + // applied afterwards. + // CHECK-FIXES-NEXT: {{^}$}} + // CHECK-FIXES-NEXT: {{^}$}} + // Following whitespace is assumed to not to belong to the else branch. + // However the check is not possible with CHECK-FIXES-NEXT. + // CHECK-FIXES: {{^}} test();{{$}} + + if (cond("preserve empty lines")) + + + int s; + + + else + + + int t; + + + test(); + + // CHECK-MESSAGES: :[[@LINE-14]]:{{[0-9]+}}: warning: statement should be inside braces + // CHECK-MESSAGES: :[[@LINE-9]]:{{[0-9]+}}: warning: statement should be inside braces + // CHECK-FIXES: {{^}} if (cond("preserve empty lines")) {{{$}} + // CHECK-FIXES-NEXT: {{^ $}} + // CHECK-FIXES-NEXT: {{^ $}} + // CHECK-FIXES-NEXT: {{^ int s;$}} + // CHECK-FIXES-NEXT: {{^ $}} + // CHECK-FIXES-NEXT: {{^ $}} + // CHECK-FIXES-NEXT: {{^ } else {$}} + // CHECK-FIXES-NEXT: {{^ $}} + // CHECK-FIXES-NEXT: {{^ $}} + // CHECK-FIXES-NEXT: {{^ int t;$}} + // The closing brace is added at beginning of line, clang-format can be + // applied afterwards. + // CHECK-FIXES-NEXT: {{^}$}} + // Following whitespace is assumed to not to belong to the else branch. + // CHECK-FIXES-NEXT: {{^ $}} + // CHECK-FIXES-NEXT: {{^ $}} + // CHECK-FIXES-NEXT: {{^}} test();{{$}} +} + +int test_return_int() { + if (cond("return5")) + return 5; + // CHECK-MESSAGES: :[[@LINE-2]]:23: warning: statement should be inside braces + // CHECK-FIXES: if (cond("return5")) { + // CHECK-FIXES-NEXT: return 5; + // CHECK-FIXES-NEXT: } + + if (cond("return{6}")) + return {6}; + // CHECK-MESSAGES: :[[@LINE-2]]:25: warning: statement should be inside braces + // CHECK-FIXES: if (cond("return{6}")) { + // CHECK-FIXES-NEXT: return {6}; + // CHECK-FIXES-NEXT: } + + // From https://bugs.llvm.org/show_bug.cgi?id=25970 + if (cond("25970")) return {25970}; + return {!25970}; + // CHECK-MESSAGES: :[[@LINE-2]]:21: warning: statement should be inside braces + // CHECK-FIXES: if (cond("25970")) { return {25970}; + // CHECK-FIXES-NEXT: } + // CHECK-FIXES-NEXT: return {!25970}; } void f(const char *p) { @@ -203,4 +374,86 @@ // CHECK-FIXES: {{^}} for (;;) {{{$}} // CHECK-FIXES-NEXT: {{^ ;$}} // CHECK-FIXES-NEXT: {{^}$}} + + + #define WRAP(X) { X; } + // This is to ensure no other CHECK-FIXES matches the macro definition: + // CHECK-FIXES: WRAP + + // Use-case: LLVM_DEBUG({ for(...) do_something(); }); + WRAP({ + for (;;) + do_something("for in wrapping macro 1"); + }); + // CHECK-MESSAGES: :[[@LINE-3]]:13: warning: statement should be inside braces + // CHECK-FIXES: for (;;) { + // CHECK-FIXES-NEXT: do_something("for in wrapping macro 1"); + // CHECK-FIXES-NEXT: } + + // Use-case: LLVM_DEBUG( for(...) do_something(); ); + WRAP( + for (;;) + do_something("for in wrapping macro 2"); + ); + // CHECK-MESSAGES: :[[@LINE-3]]:13: warning: statement should be inside braces + // CHECK-FIXES: for (;;) { + // CHECK-FIXES-NEXT: do_something("for in wrapping macro 2"); + // CHECK-FIXES-NEXT: } + + // Use-case: LLVM_DEBUG( for(...) do_something() ); + // This is not supported and this test ensure it's correctly not changed. + // We don't want to add the `}` into the Macro and there is no other way + // to add it except for introduction of a NullStmt. + WRAP( + for (;;) + do_something("for in wrapping macro 3") + ); + // CHECK-MESSAGES: :[[@LINE-3]]:13: warning: statement should be inside braces + // CHECK-FIXES: WRAP( + // CHECK-FIXES-NEXT: for (;;) + // CHECK-FIXES-NEXT: do_something("for in wrapping macro 3") + // CHECK-FIXES-NEXT: ); + + // Taken from https://bugs.llvm.org/show_bug.cgi?id=22785 + int i; + #define MACRO_1 i++ + #define MACRO_2 + if( i % 3) i--; + else if( i % 2) MACRO_1; + else MACRO_2; + // CHECK-MESSAGES: :[[@LINE-3]]:13: warning: statement should be inside braces + // CHECK-MESSAGES: :[[@LINE-3]]:18: warning: statement should be inside braces + // CHECK-MESSAGES: :[[@LINE-3]]:7: warning: statement should be inside braces + // CHECK-FIXES: if( i % 3) { i--; + // CHECK-FIXES-NEXT: } else if( i % 2) { MACRO_1; + // CHECK-FIXES-NEXT: } else { MACRO_2; + // CHECK-FIXES-NEXT: } + + // Taken from https://bugs.llvm.org/show_bug.cgi?id=22785 + #define M(x) x + + if (b) + return 1; + else + return 2; + // CHECK-MESSAGES: :[[@LINE-4]]:9: warning: statement should be inside braces + // CHECK-MESSAGES: :[[@LINE-3]]:7: warning: statement should be inside braces + // CHECK-FIXES: if (b) { + // CHECK-FIXES-NEXT: return 1; + // CHECK-FIXES-NEXT: } else { + // CHECK-FIXES-NEXT: return 2; + // CHECK-FIXES-NEXT: } + + if (b) + return 1; + else + M(return 2); + // CHECK-MESSAGES: :[[@LINE-4]]:9: warning: statement should be inside braces + // CHECK-MESSAGES: :[[@LINE-3]]:7: warning: statement should be inside braces + // CHECK-FIXES: if (b) { + // CHECK-FIXES-NEXT: return 1; + // CHECK-FIXES-NEXT: } else { + // CHECK-FIXES-NEXT: M(return 2); + // CHECK-FIXES-NEXT: } + }