Handle C++11 brace initializers in readability-braces-around-statements

[adek05]

Summary

C++11 brace initialization messes up with a condition which is used to check whether we are past the end of the statement intended to be enclosed with braces. Fix looks one token ahead, to check whether we are in a situation of "};" which could suggest a brace initialization is used and shoud work fine in other cases.

[adek05]

clang-tidy/readability/BracesAroundStatementsCheck.cpp

63–72

This might not be the best approach. Maybe it will be better to try to find the matching opening brace and see whether it is the opening brace for 'then' block. Feel free to throw more suggestions.

[alexfh]

clang-tidy/readability/BracesAroundStatementsCheck.cpp

63–72

Maybe just skip correct bracket sequences (a statement can also contain lambdas, subscript expressions and normal parentheses in various combinations)?

[adek05]

clang-tidy/readability/BracesAroundStatementsCheck.cpp

63–72

Well, we could just remove TokKind == tok::r_brace in line 64. I don't really understand why this check is there. The test suite passes if I remove that check, including the new cases I have added. Maybe that's the way to go?

I tracked the initial commit which introduced TokKind == tok::r_brace: D5395 and I don't see a clear reason why it needs to be there. Do you know why we need it?

[alexfh]

clang-tidy/readability/BracesAroundStatementsCheck.cpp

63–72

Well, we could just remove TokKind == tok::r_brace in line 64.

You mean line 74, I guess. It should be easy to check if removing it helps by running tests and applying fixes to a large enough codebase, e.g. LLVM+Clang, and running its tests to ensure the correctness of changes + manual inspection of a few replacements.

[AlexanderLanin]

Here is the r_brace use case:

if (true) while ("ok") {}

Unfortunately it looks quite similar to:

if (true) s = {};

clang-tidy/readability/BracesAroundStatementsCheck.cpp

SourceLocation findEndLocation(SourceLocation LastTokenLoc,
                                const SourceManager &SM,
                                const ASTContext *Context) {
   SourceLocation Loc = LastTokenLoc;
   // 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 we are at "}", but the following token is ";", then we could be
+  // reading a statement like "x = A{};" and "}" does not mean we reached
+  // the end of a statement. Hence we look ahead.
+  SourceLocation LocOneAhead = forwardSkipWhitespaceAndComments(Loc.getLocWithOffset(1), SM, Context);
+  tok::TokenKind OneAheadTokKind;
+  if (LocOneAhead.isValid()) {
+    OneAheadTokKind = getTokenKind(LocOneAhead, SM, Context);
+  } else {
+    OneAheadTokKind = tok::NUM_TOKENS;
+  }
   if (TokKind == tok::NUM_TOKENS || TokKind == tok::semi ||
-      TokKind == tok::r_brace) {
+      (TokKind == tok::r_brace && OneAheadTokKind != tok::semi)) {
     // 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;
   }
 
   Loc = Lexer::getLocForEndOfToken(Loc, 0, SM, Context->getLangOpts());
   // Loc points past the last token before end or after ';'.
 

test/clang-tidy/readability-braces-around-statements.cpp

-// RUN: %check_clang_tidy %s readability-braces-around-statements %t
+// RUN: %check_clang_tidy %s readability-braces-around-statements %t -- -- -std=c++11
 
 void do_something(const char *) {}
 
 bool cond(const char *) {
   return false;
 }
 
 #define EMPTY_MACRO
 #define EMPTY_MACRO_FUN()
 
 void test() {
   if (cond("if0") /*comment*/) do_something("same-line");
   // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: statement should be inside braces
   // CHECK-FIXES:   if (cond("if0") /*comment*/) { do_something("same-line");
   // CHECK-FIXES: }
 
   if (cond("if0.1"))
     do_something("single-line");
   // CHECK-MESSAGES: :[[@LINE-2]]:21: warning: statement should be inside braces
   // CHECK-FIXES: if (cond("if0.1")) {
   // CHECK-FIXES: }
 
+  int s;
+  if (cond("c++11 initialization"))
+    s = int{};
+  // CHECK-MESSAGES: :[[@LINE-2]]:36: warning: statement should be inside braces
+  // CHECK-FIXES: if (cond("c++11 initialization")) {
+  // CHECK-FIXES: }
+
+  for (;;)
+    s = int{};
+  // CHECK-MESSAGES: :[[@LINE-2]]:11: warning: statement should be inside braces
+  // CHECK-FIXES: for (;;)
+  // CHECK-FIXES: }
+
   if (cond("if1") /*comment*/)
     // some comment
     do_something("if1");
   // CHECK-MESSAGES: :[[@LINE-3]]:31: warning: statement should be inside braces
   // CHECK-FIXES: if (cond("if1") /*comment*/) {
   // CHECK-FIXES: }
   if (cond("if2")) {
     do_something("if2");