[clang-tidy] fix readability-braces-around-statements Stmt type dependency

[AlexanderLanin]

Summary

Replaces Token based approach to identify EndLoc of Stmt with AST traversal.
This also improves handling of macros.

Fixes Bugs 22785, 25970 and 35754.

[AlexanderLanin]

Oh sorry. It's crashing. I'll look into it.

[AlexanderLanin]

1. Extended Test for another macro scenario.
2. Added graceful termination to handle problematic locations.
3. Changed Lexer::findNextToken to no longer run into that graceful termination. Test is passing. I'm not sure whether that's a good idea? check-clang and check-clang-tools are passing with this change.

[AlexanderLanin]

clang-tools-extra/clang-tidy/utils/LexerUtils.h

112–113

Maybe this should be more in the line of getStmtLikeEndLoc?
With a note "This will fail horribly if Stmt is not Stmt-like" - whatever that means ;-)

Compare http://lists.llvm.org/pipermail/cfe-dev/2020-March/064844.html

Here is the rest of the mail thread: http://lists.llvm.org/pipermail/cfe-dev/2020-February/064743.html

[AlexanderLanin]

• Added GotoStmt and SEHLeaveStmt as learned from D76108.
• After applying D76037 and D76054 I was able to run-clang-tidy on llvm and therefore found some new macro related issues

[AlexanderLanin]

Accidentally reverted test file renaming in last commit. Fixed now.
Rebased on current master.

[AlexanderLanin]

clang-tools-extra/clang-tidy/utils/LexerUtils.h

112–113

Sorry this marked itself as Done.
I'd still like feedback on the name.
getStmtLikeEndLoc? getEndLocInclSemi?

[AlexanderLanin]

Add further tests from more bug reports.

[njames93]

Can you add a test case when the else body ends with braces. In one of the bug reports that case was mentioned

else
  X = {6};

clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements.cpp

86

As the old behaviour didn't use check fixes next is hard to tell how the semi colon was handled for null stmt. In the old version were they kept when brackets were added, or removed. If they were removed I'd prefer that behaviour to stay. If you just made the test more explicit, disregard this.

[AlexanderLanin]

clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements.cpp

86

It's not new behavior. Although now that you mention it I'm not sure it's good behavior. However I have not modified it.

I found that some of the tests here did not actually test what they were supposed to. For example this test was matching against 'do_something("for");' instead of the NullStmt input. That's why I modified several either with unique strings or with -NEXT.

[AlexanderLanin]

new test case as requested (line 256)

[njames93]

clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements.cpp

86

Fair enough. I think the way NullStmt is handled by this check isn't great. But I don't know what the preferred way would be?

• No braces added
• Braces add and semi-colon removed
• Braces added, semi-colon kept
• Just add an option to let users specify behaviour

In any case it's out of scope for this revision.

[AlexanderLanin]

*ping*

I have an opinion on NullStmt, but I would rather see this merged ;-)
Behavior is same as before.

[AlexanderLanin]

*ping*

[AlexanderLanin]

*ping*

[AlexanderLanin]

*ping*
Sorry, but I have to ask: Is there no interest in improving clang-tidy?
Don't get me wrong. I know it's mostly volunteered time. That's also why this ping took almost 2 months for me.
I frankly lost interest in contributing as I have trouble getting any patches included - and I don't have that much free time anymore. Obviously the time issue is more important, but the other one is still a problem.

[njames93]

In D75813#2183198, @AlexanderLanin wrote:

*ping*
Sorry, but I have to ask: Is there no interest in improving clang-tidy?
Don't get me wrong. I know it's mostly volunteered time. That's also why this ping took almost 2 months for me.
I frankly lost interest in contributing as I have trouble getting any patches included - and I don't have that much free time anymore. Obviously the time issue is more important, but the other one is still a problem.

I agree this needs fixing too, Added a few more reviewers to help move this along

clang-tools-extra/clang-tidy/utils/LexerUtils.cpp

154

llvm::isa can take multiple types to check for any matching. Same goes for below

156

nit: new line between functions, same below

209–213

nit: remove braces and empty line

clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements.cpp

99

Can you explicitly spell the column rather than just regexing for any number, helps prevent any changes down the line altering where the warning is displayed by accident.

213

Does this line need to be here?

[AlexanderLanin]

Addressed findings by @njames93

[AlexanderLanin]

clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements.cpp

99

"where the warning is displayed by accident"... well it did check the line number. But I've extended with column number as requested.

213

Yes and no.
You are correct in that it doesn't check for any change. Except maybe that no second brace is added.
However what it does is ensuring that the following CHECK-FIXES is applied after cond("break and continue"). This can surely be solved differently, but currently the next CHECK-FIXES is a rather generic if(true) which could be anywhere in code. This plus the // CHECK-FIXES: } //end sets the scope for the CHECK-FIXES in between. The // end comment is there to ensure that not some other random } is matched.

[njames93]

clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements.cpp

213

Ahh fair enough.
However I don't think it will check to ensure a second brace wasn't added unless you use an eol anchor on the line {{$}} due to how FileCheck works.

[AlexanderLanin]

Rebased and stored at https://github.com/llvm/llvm-project/compare/9d6d4b07a...AlexanderLanin:readability-braces-around-statements?expand=1 so I have a branch next time I need to rebase :-)

Still looking for someone to approve and to commit this change.

[JonasToth]

Hey Alex, first thanks for the bug fixing! Of course we are interested in improvements in clang-tidy, but i think noone is actually employed to help out here and the normal reviewers have many, many reviews parallel. Sometimes stuff just slips through :/

I try to pick up review an @njames93 seems not have time (i dont have info, and you can just jump in again!).

Two thoughts are added inline. Did you check the new revision against a big code-base like llvm (with fixit on and compilation + testing afterwards)? How did it go, anything outstanding happened?

clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp

258

Why is this assertion removed? In the following lines EndLoc is used for a line-number calculation. Shouldn't EndLoc be valid for that to be correct?

clang-tools-extra/clang-tidy/utils/LexerUtils.h

112–113

If personally think that getUnifiedEndLoc is ok. LexerUtils.h is not super exposed and the name makes the point.

[AlexanderLanin]

@JonasToth I fully realize that it's voluntary and I do not have a solution. I have the same problem in a different area. Not sure what shall become of some OSS projects when it takes that long to get a patch in. But that's a different discussion.

The patch as shown here generates ~130k braces on llvm, you can see it at https://github.com/llvm/llvm-project/commit/16d3ec868cf00

• The braces are not reviewed
• check-all still compiles
• check-all has the exact same results as before the auto-fix

It's not tested against other codebases, but it had a test suite even before this patch.

I'll get back regarding that removed assert ASAP. I don't remember, so I'll add it again and see what happens.

[njames93]

In D75813#2326060, @AlexanderLanin wrote:

The patch as shown here generates ~130k braces on llvm, you can see it at https://github.com/llvm/llvm-project/commit/16d3ec868cf00

That diff is showing different behaviour to the test cases. In the test cases

if (...)
  something();

Will get changed to

if (...) {
  something();
}

However in the diff it is inserting a new line before the closing brace

if (...) {
  something();

}

This somewhat seems to decrease readability.
It seems that for an if/else only the new line is inserted in the else branch.

[AlexanderLanin]

@JonasToth: Cannot add assert. EndLoc is indeed invalid in case BeginLoc is outside the macro and EndLoc would be inside the macro. I've added a comment explaining that.

@njames93: Sorry I generated the diff for github upload incorrectly. Yaml newline format has changed between release 11 and 12. One has to provide the new clang-apply-replacements-binary to run-clang-tidy.
Anyway I've added test cases to check for such newlines to prevent regressions.
Fixed diff (only partial this time, only newlines changed): https://github.com/AlexanderLanin/llvm-project/commit/a6cae954e95

[njames93]

LGTM

clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements.cpp

274–275

nit: Hard code the column numbers.

279

While its entirely possible to fix this using the lexer to get indentation, Perfectly happy to leave this job for clang-format. Besides it is needed to handle how a user wants to wrap braces anyway.

283–284

have you tried using the CHECK_EMPTY Directive?

301–302

nit: Hard code the column numbers.

[AlexanderLanin]

It's been a while and I almost don't remember what this is. But it was a good fix and there is one LGTM.
Can someone please commit this? Alexander Lanin <[e-mail]>
Thanks!

[aaron.ballman]

In D75813#2640789, @AlexanderLanin wrote:

It's been a while and I almost don't remember what this is. But it was a good fix and there is one LGTM.
Can someone please commit this? Alexander Lanin <[e-mail]>
Thanks!

I've commit on your behalf in 0becc4d721d0036e2e38d581bc487e27f78eb8a9, thanks!

clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp

 //===--- BracesAroundStatementsCheck.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
 //
 //===----------------------------------------------------------------------===//
 
 #include "BracesAroundStatementsCheck.h"
+#include "../utils/LexerUtils.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Lex/Lexer.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace readability {
-namespace {
 
-tok::TokenKind getTokenKind(SourceLocation Loc, const SourceManager &SM,
+static tok::TokenKind getTokenKind(SourceLocation Loc, const SourceManager &SM,
                                    const ASTContext *Context) {
   Token Tok;
   SourceLocation Beginning =
       Lexer::GetBeginningOfToken(Loc, SM, Context->getLangOpts());
   const bool Invalid =
       Lexer::getRawToken(Beginning, Tok, SM, Context->getLangOpts());
   assert(!Invalid && "Expected a valid token.");
 
   if (Invalid)
     return tok::NUM_TOKENS;
 
   return Tok.getKind();
 }
 
-SourceLocation forwardSkipWhitespaceAndComments(SourceLocation Loc,
-                                                const SourceManager &SM,
-                                                const ASTContext *Context) {
+static SourceLocation
+forwardSkipWhitespaceAndComments(SourceLocation Loc, const SourceManager &SM,
   assert(Loc.isValid());
   for (;;) {
     while (isWhitespace(*SM.getCharacterData(Loc)))
       Loc = Loc.getLocWithOffset(1);
 
     tok::TokenKind TokKind = getTokenKind(Loc, SM, Context);
     if (TokKind != tok::comment)
       return Loc;
 
     // Fast-forward current token.
     Loc = Lexer::getLocForEndOfToken(Loc, 0, SM, Context->getLangOpts());
   }
 }
 
-SourceLocation findEndLocation(SourceLocation LastTokenLoc,
-                               const SourceManager &SM,
+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<NullStmt>(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());
     while (isHorizontalWhitespace(*SM.getCharacterData(Loc))) {
       Loc = Loc.getLocWithOffset(1);
     }
 
     if (isVerticalWhitespace(*SM.getCharacterData(Loc))) {
for (;;) {
     // else: Trailing comment, insert brace after the newline.
 
     // Fast-forward current token.
     Loc = TokEndLoc;
   }
   return Loc;
 }
 
-} // namespace
-
 BracesAroundStatementsCheck::BracesAroundStatementsCheck(
     StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       // Always add braces by default.
       ShortStatementLines(Options.get("ShortStatementLines", 0U)) {}
 
 void BracesAroundStatementsCheck::storeOptions(
     ClangTidyOptions::OptionMap &Opts) {
if (!S || isa<CompoundStmt>(S)) {
     return false;
   }
 
   if (!InitialLoc.isValid())
     return false;
   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.
   InitialLoc = Lexer::makeFileCharRange(
                    CharSourceRange::getCharRange(InitialLoc, S->getBeginLoc()),
                    SM, Context->getLangOpts())
                    .getBegin();
   if (InitialLoc.isInvalid())
     return false;
   SourceLocation StartLoc =
       Lexer::getLocForEndOfToken(InitialLoc, 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 {
-    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)) {
     unsigned StartLine = SM.getSpellingLineNumber(StartLoc);
     unsigned EndLine = SM.getSpellingLineNumber(EndLoc);
     if (EndLine - StartLine < ShortStatementLines)
       return false;
   }
 
   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;
 }
 
 void BracesAroundStatementsCheck::onEndOfTranslationUnit() {
   ForceBracesStmts.clear();
 }
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang

clang-tools-extra/clang-tidy/utils/LexerUtils.h

 //===--- LexerUtils.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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_LEXER_UTILS_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_LEXER_UTILS_H
 
 #include "clang/AST/ASTContext.h"
 #include "clang/Basic/TokenKinds.h"
 #include "clang/Lex/Lexer.h"
 
 namespace clang {
+
+class Stmt;
+
 namespace tidy {
 namespace utils {
 namespace lexer {
 
 /// Returns previous token or ``tok::unknown`` if not found.
 Token getPreviousToken(SourceLocation Location, const SourceManager &SM,
                        const LangOptions &LangOpts, bool SkipComments = true);
 

 /// must be valid with respect to ``SM``.  Returns ``None`` if no qualifying
 /// tokens are found.
 /// \note: doesn't support member function qualifiers.
 llvm::Optional<Token> getQualifyingToken(tok::TokenKind TK,
                                          CharSourceRange Range,
                                          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
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_LEXER_UTILS_H

clang-tools-extra/clang-tidy/utils/LexerUtils.cpp

 //===--- LexerUtils.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
 //
 //===----------------------------------------------------------------------===//
 
 #include "LexerUtils.h"
+#include "clang/AST/AST.h"
 #include "clang/Basic/SourceManager.h"
 
 namespace clang {
 namespace tidy {
 namespace utils {
 namespace lexer {
 
 Token getPreviousToken(SourceLocation Location, const SourceManager &SM,
else if (Tok.is(TK)) {
         LastMatchAfterTemplate = Tok;
       else
         LastMatchBeforeTemplate = Tok;
     }
   }
   return LastMatchAfterTemplate != None ? LastMatchAfterTemplate
                                         : LastMatchBeforeTemplate;
 }
+
+static bool breakAndReturnEnd(const Stmt &S) {
+  return isa<CompoundStmt, DeclStmt, NullStmt>(S);
+}
+
+static bool breakAndReturnEndPlus1Token(const Stmt &S) {
+  return isa<Expr, DoStmt, ReturnStmt, BreakStmt, ContinueStmt, GotoStmt, SEHLeaveStmt>(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<Token> 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<Token> 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
 } // namespace clang

clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements.cpp

void test() {
   } else {
     do_something("else");
   }
 
   for (;;)
     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");
   // CHECK-MESSAGES: :[[@LINE-2]]:25: warning: statement should be inside braces
   // CHECK-FIXES: while (cond("while1")) {
   // CHECK-FIXES: }
   while (cond("while2")) {
     do_something("while");
void test() {
   // CHECK-MESSAGES: :[[@LINE-5]]:21: warning: statement should be inside braces
   // CHECK-MESSAGES: :[[@LINE-4]]:23: warning: statement should be inside braces
   // CHECK-FIXES: if (cond("ifif1")) {
   // CHECK-FIXES: if (cond("ifif2")) {
   // CHECK-FIXES: }
   // 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
         comment */
   // CHECK-MESSAGES: :[[@LINE-3]]:21: warning: statement should be inside braces
   // CHECK-FIXES: if (cond("ifif5")) {
   // CHECK-FIXES: }/* multi-line
 
   if (1) while (2) if (3) for (;;) do ; while(false) /**/;/**/
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: statement should be inside braces
   // CHECK-MESSAGES: :[[@LINE-2]]:19: warning: statement should be inside braces
   // CHECK-MESSAGES: :[[@LINE-3]]:26: warning: statement should be inside braces
   // CHECK-MESSAGES: :[[@LINE-4]]:35: warning: statement should be inside braces
   // CHECK-MESSAGES: :[[@LINE-5]]:38: warning: statement should be inside braces
   // CHECK-FIXES: if (1) { while (2) { if (3) { for (;;) { do { ; } while(false) /**/;/**/
   // CHECK-FIXES-NEXT: }
   // 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) {
   if (!p)
     f("\
 ");
 } // end of f
 // CHECK-MESSAGES: :[[@LINE-4]]:10: warning: statement should be inside braces
int test_macros(bool b) {
   M(
     for (;;)
       ;
   );
   // CHECK-MESSAGES: :[[@LINE-3]]:13: warning: statement should be inside braces
   // 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: }
+
 }