[clang-format] Handle do-while loops for RemoveBracesLLVM

[owenpan]

Summary

If a do-while loop is in the block of a control statement, do not remove the braces.

Also updates the unit tests to match the updated LLVM Coding Standards. (See D126512.)

[curdeius]

clang/unittests/Format/FormatTest.cpp

25103

Nit. To avoid updating it every time we add an example.

25407–25411

What's the value of testing that newlines are added? That's not really the job of Insert/RemoveBraces.

[MyDeveloperDay]

LGTM

[owenpan]

clang/unittests/Format/FormatTest.cpp

25103

Yep. I will also remove the numbering on the examples.

25407–25411

This was from the actual llvm-project source. I used it not because I wanted to test the insertion of newlines, but because I fixed some insert/remove braces bugs related to whether the braces were wrapped.

[owenpan]

Removed the numbering of the examples from the coding standards.

[HazardyKnusperkeks]

clang/lib/Format/UnwrappedLineParser.h

118–119

Should we merge that to something like KeepBraces? Or do we use HasLabel and HasDoWhile differently?

[owenpan]

clang/lib/Format/UnwrappedLineParser.h

118–119

We will use them differently. There will be no configuration for labels but some configurations for do-while loops eventually.

[HazardyKnusperkeks]

clang/lib/Format/UnwrappedLineParser.h

118–119

Ok.

clang/lib/Format/UnwrappedLineParser.h

private:
   void parsePPElIf();
   void parsePPElse();
   void parsePPEndIf();
   void parsePPUnknown();
   void readTokenWithJavaScriptASI();
   void parseStructuralElement(IfStmtKind *IfKind = nullptr,
                               bool IsTopLevel = false,
                               TokenType NextLBracesType = TT_Unknown,
+                              bool *HasDoWhile = nullptr,
                               bool *HasLabel = nullptr);
   bool tryToParseBracedList();
   bool parseBracedList(bool ContinueOnSemicolons = false, bool IsEnum = false,
                        tok::TokenKind ClosingBraceKind = tok::r_brace);
   void parseParens(TokenType AmpAmpTokenType = TT_Unknown);
   void parseSquare(bool LambdaIntroducer = false);
   void keepAncestorBraces();
   void parseUnbracedBody(bool CheckEOF = false);
   void handleAttributes();

clang/lib/Format/UnwrappedLineParser.cpp

bool UnwrappedLineParser::parseLevel(const FormatToken *OpeningBrace,
                                      bool CanContainBracedList,
                                      IfStmtKind *IfKind,
                                      TokenType NextLBracesType) {
   auto NextLevelLBracesType = NextLBracesType == TT_CompoundRequirementLBrace
                                   ? TT_BracedListLBrace
                                   : TT_Unknown;
   const bool IsPrecededByCommentOrPPDirective =
       !Style.RemoveBracesLLVM || precededByCommentOrPPDirective();
+  bool HasDoWhile = false;
   bool HasLabel = false;
   unsigned StatementCount = 0;
   bool SwitchLabelEncountered = false;
   do {
     if (FormatTok->getType() == TT_AttributeMacro) {
       nextToken();
       continue;
     }
     tok::TokenKind kind = FormatTok->Tok.getKind();
     if (FormatTok->getType() == TT_MacroBlockBegin)
       kind = tok::l_brace;
     else if (FormatTok->getType() == TT_MacroBlockEnd)
       kind = tok::r_brace;
 
     auto ParseDefault = [this, OpeningBrace, IfKind, NextLevelLBracesType,
-                         &HasLabel, &StatementCount] {
+                         &HasDoWhile, &HasLabel, &StatementCount] {
       parseStructuralElement(IfKind, !OpeningBrace, NextLevelLBracesType,
+                             HasDoWhile ? nullptr : &HasDoWhile,
                              HasLabel ? nullptr : &HasLabel);
       ++StatementCount;
       assert(StatementCount > 0 && "StatementCount overflow!");
     };
 
     switch (kind) {
     case tok::comment:
       nextToken();
case tok::l_brace:
       break;
     case tok::r_brace:
       if (OpeningBrace) {
         if (!Style.RemoveBracesLLVM ||
             !OpeningBrace->isOneOf(TT_ControlStatementLBrace, TT_ElseLBrace)) {
           return false;
         }
         if (FormatTok->isNot(tok::r_brace) || StatementCount != 1 || HasLabel ||
-            IsPrecededByCommentOrPPDirective ||
+            HasDoWhile || IsPrecededByCommentOrPPDirective ||
             precededByCommentOrPPDirective()) {
           return false;
         }
         const FormatToken *Next = Tokens->peekNextToken();
         return Next->isNot(tok::comment) || Next->NewlinesBefore > 0;
       }
       nextToken();
       addUnwrappedLine();
if ((PreviousMustBeValue || Previous->is(tok::r_paren)) &&
       isJSDeclOrStmt(Keywords, Next)) {
     return addUnwrappedLine();
   }
 }
 
 void UnwrappedLineParser::parseStructuralElement(IfStmtKind *IfKind,
                                                  bool IsTopLevel,
                                                  TokenType NextLBracesType,
+                                                 bool *HasDoWhile,
                                                  bool *HasLabel) {
   if (Style.Language == FormatStyle::LK_TableGen &&
       FormatTok->is(tok::pp_include)) {
     nextToken();
     if (FormatTok->is(tok::string_literal))
       nextToken();
     addUnwrappedLine();
     return;
case tok::kw_while:
     parseForOrWhileLoop();
     return;
   case tok::kw_do:
     if (Style.isJavaScript() && Line->MustBeDeclaration) {
       // field/method declaration.
       break;
     }
     parseDoWhile();
+    if (HasDoWhile)
+      *HasDoWhile = true;
     return;
   case tok::kw_switch:
     if (Style.isJavaScript() && Line->MustBeDeclaration) {
       // 'switch: string' field declaration.
       break;
     }
     parseSwitch();
     return;

clang/unittests/Format/FormatTest.cpp

verifyFormat("if (a + b >\n"
                "  f();",
                Style);
 }
 
 TEST_F(FormatTest, RemoveBraces) {
   FormatStyle Style = getLLVMStyle();
   Style.RemoveBracesLLVM = true;
 
-  // The following eight test cases are fully-braced versions of the examples at
+  // The following nine test cases are fully-braced versions of the examples at
   // "llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-
   // statement-bodies-of-if-else-loop-statements".
 
-  // 1. Omit the braces, since the body is simple and clearly associated with
-  // the if.
+  // 1. Omit the braces since the body is simple and clearly associated with the
+  // `if`.
   verifyFormat("if (isa<FunctionDecl>(D))\n"
                "  handleFunctionDecl(D);\n"
                "else if (isa<VarDecl>(D))\n"
                "  handleVarDecl(D);",
                "if (isa<FunctionDecl>(D)) {\n"
                "  handleFunctionDecl(D);\n"
                "} else if (isa<VarDecl>(D)) {\n"
                "  handleVarDecl(D);\n"
                "}",
                Style);
 
   // 2. Here we document the condition itself and not the body.
   verifyFormat("if (isa<VarDecl>(D)) {\n"
                "  // It is necessary that we explain the situation with this\n"
                "  // surprisingly long comment, so it would be unclear\n"
                "  // without the braces whether the following statement is in\n"
                "  // the scope of the `if`.\n"
                "  // Because the condition is documented, we can't really\n"
                "  // hoist this comment that applies to the body above the\n"
-               "  // if.\n"
+               "  // `if`.\n"
                "  handleOtherDecl(D);\n"
                "}",
                Style);
 
-  // 3. Use braces on the outer `if` to avoid a potential dangling else
+  // 3. Use braces on the outer `if` to avoid a potential dangling `else`
   // situation.
   verifyFormat("if (isa<VarDecl>(D)) {\n"
-               "  for (auto *A : D.attrs())\n"
-               "    if (shouldProcessAttr(A))\n"
-               "      handleAttr(A);\n"
+               "  if (shouldProcessAttr(A))\n"
+               "    handleAttr(A);\n"
                "}",
                "if (isa<VarDecl>(D)) {\n"
-               "  for (auto *A : D.attrs()) {\n"
-               "    if (shouldProcessAttr(A)) {\n"
-               "      handleAttr(A);\n"
-               "    }\n"
-               "  }\n"
+               "  if (shouldProcessAttr(A)) {\n"
+               "    handleAttr(A);\n"
+               "  }\n"
                "}",
                Style);
 
-  // 4. Use braces for the `if` block to keep it uniform with the else block.
+  // 4. Use braces for the `if` block to keep it uniform with the `else` block.
   verifyFormat("if (isa<FunctionDecl>(D)) {\n"
                "  handleFunctionDecl(D);\n"
                "} else {\n"
-               "  // In this else case, it is necessary that we explain the\n"
+               "  // In this `else` case, it is necessary that we explain the\n"
                "  // situation with this surprisingly long comment, so it\n"
                "  // would be unclear without the braces whether the\n"
                "  // following statement is in the scope of the `if`.\n"
                "  handleOtherDecl(D);\n"
                "}",
                Style);
 
-  // 5. This should also omit braces.  The `for` loop contains only a single
+  // 5. This should also omit braces. The `for` loop contains only a single
   // statement, so it shouldn't have braces.  The `if` also only contains a
-  // single simple statement (the for loop), so it also should omit braces.
+  // single simple statement (the `for` loop), so it also should omit braces.
   verifyFormat("if (isa<FunctionDecl>(D))\n"
                "  for (auto *A : D.attrs())\n"
                "    handleAttr(A);",
                "if (isa<FunctionDecl>(D)) {\n"
                "  for (auto *A : D.attrs()) {\n"
                "    handleAttr(A);\n"
                "  }\n"
                "}",
                Style);
 
-  // 6. Use braces for the outer `if` since the nested `for` is braced.
+  // 6. Use braces for a `do-while` loop and its enclosing statement.
+  verifyFormat("if (Tok->is(tok::l_brace)) {\n"
+               "  do {\n"
+               "    Tok = Tok->Next;\n"
+               "  } while (Tok);\n"
+               "}",
+               Style);
+
+  // 7. Use braces for the outer `if` since the nested `for` is braced.
   verifyFormat("if (isa<FunctionDecl>(D)) {\n"
                "  for (auto *A : D.attrs()) {\n"
-               "    // In this for loop body, it is necessary that we explain\n"
-               "    // the situation with this surprisingly long comment,\n"
-               "    // forcing braces on the `for` block.\n"
+               "    // In this `for` loop body, it is necessary that we\n"
+               "    // explain the situation with this surprisingly long\n"
+               "    // comment, forcing braces on the `for` block.\n"
                "    handleAttr(A);\n"
                "  }\n"
                "}",
                Style);
 
-  // 7. Use braces on the outer block because there are more than two levels of
+  // 8. Use braces on the outer block because there are more than two levels of
   // nesting.
   verifyFormat("if (isa<FunctionDecl>(D)) {\n"
                "  for (auto *A : D.attrs())\n"
                "    for (ssize_t i : llvm::seq<ssize_t>(count))\n"
                "      handleAttrOnDecl(D, A, i);\n"
                "}",
                "if (isa<FunctionDecl>(D)) {\n"
                "  for (auto *A : D.attrs()) {\n"
                "    for (ssize_t i : llvm::seq<ssize_t>(count)) {\n"
                "      handleAttrOnDecl(D, A, i);\n"
                "    }\n"
                "  }\n"
                "}",
                Style);
 
-  // 8. Use braces on the outer block because of a nested `if`, otherwise the
+  // 9. Use braces on the outer block because of a nested `if`; otherwise the
   // compiler would warn: `add explicit braces to avoid dangling else`
   verifyFormat("if (auto *D = dyn_cast<FunctionDecl>(D)) {\n"
                "  if (shouldProcess(D))\n"
                "    handleVarDecl(D);\n"
                "  else\n"
                "    markAsIgnored(D);\n"
                "}",
                "if (auto *D = dyn_cast<FunctionDecl>(D)) {\n"
verifyFormat("if (a) {\n"
                "    if (d)\n"
                "      e;\n"
                "  }\n"
                "} else {\n"
                "  g;\n"
                "}",
                Style);
 
+  verifyFormat("if (isa<VarDecl>(D)) {\n"
+               "  for (auto *A : D.attrs())\n"
+               "    if (shouldProcessAttr(A))\n"
+               "      handleAttr(A);\n"
+               "}",
+               "if (isa<VarDecl>(D)) {\n"
+               "  for (auto *A : D.attrs()) {\n"
+               "    if (shouldProcessAttr(A)) {\n"
+               "      handleAttr(A);\n"
+               "    }\n"
+               "  }\n"
+               "}",
+               Style);
+
+  verifyFormat("do {\n"
+               "  ++I;\n"
+               "} while (hasMore() && Filter(*I));",
+               "do { ++I; } while (hasMore() && Filter(*I));",
+               Style);
+
   verifyFormat("if (a)\n"
                "  if (b)\n"
                "    c;\n"
                "  else {\n"
                "    if (d)\n"
                "      e;\n"
                "  }\n"
                "else\n"