[clang-format] Handle "complex" conditionals in RemoveBracesLLVM

[owenpan]

Summary

Do not remove braces if the conditional of if/for/while might not fit on a single line even after the opening brace is removed.

Examples:

// ColumnLimit: 20
// 45678901234567890
if (a) { /* Remove. */
  foo();
}
if (-b >= c) { // Keep.
  bar();
}

This is the last feature/exception of RemoveBracesLLVM.

[curdeius]

LGTM

[owenpan]

Changed an unnecessary condition to an assertion.

clang/lib/Format/UnwrappedLineParser.h

private:
   };
 
   void reset();
   void parseFile();
   bool precededByCommentOrPPDirective() const;
   bool parseLevel(const FormatToken *OpeningBrace, bool CanContainBracedList,
                   IfStmtKind *IfKind = nullptr,
                   TokenType NextLBracesType = TT_Unknown);
-  bool mightFitOnOneLine(UnwrappedLine &Line) const;
+  bool mightFitOnOneLine(UnwrappedLine &Line,
+                         const FormatToken *OpeningBrace = nullptr) const;
   IfStmtKind parseBlock(bool MustBeDeclaration = false, unsigned AddLevels = 1u,
                         bool MunchSemi = true, bool KeepBraces = true,
                         bool UnindentWhitesmithsBraces = false,
                         bool CanContainBracedList = true,
                         TokenType NextLBracesType = TT_Unknown);
   void parseChildBlock(bool CanContainBracedList = true,
                        TokenType NextLBracesType = TT_Unknown);
   void parsePPDirective();

clang/lib/Format/UnwrappedLineParser.cpp

size_t UnwrappedLineParser::computePPHash() const {
   size_t h = 0;
   for (const auto &i : PPStack) {
     hash_combine(h, size_t(i.Kind));
     hash_combine(h, i.Line);
   }
   return h;
 }
 
-// Checks whether \p ParsedLine might fit on a single line. We must clone the
-// tokens of \p ParsedLine before running the token annotator on it so that we
-// can restore them afterward.
-bool UnwrappedLineParser::mightFitOnOneLine(UnwrappedLine &ParsedLine) const {
+// Checks whether \p ParsedLine might fit on a single line. If \p OpeningBrace
+// is not null, subtracts its length (plus the preceding space) when computing
+// the length of \p ParsedLine. We must clone the tokens of \p ParsedLine before
+// running the token annotator on it so that we can restore them afterward.
+bool UnwrappedLineParser::mightFitOnOneLine(
+    UnwrappedLine &ParsedLine, const FormatToken *OpeningBrace) const {
   const auto ColumnLimit = Style.ColumnLimit;
   if (ColumnLimit == 0)
     return true;
 
   auto &Tokens = ParsedLine.Tokens;
   assert(!Tokens.empty());
+
   const auto *LastToken = Tokens.back().Tok;
   assert(LastToken);
 
   SmallVector<UnwrappedLineNode> SavedTokens(Tokens.size());
 
   int Index = 0;
   for (const auto &Token : Tokens) {
     assert(Token.Tok);
     auto &SavedToken = SavedTokens[Index++];
     SavedToken.Tok = new FormatToken;
     SavedToken.Tok->copyFrom(*Token.Tok);
     SavedToken.Children = std::move(Token.Children);
   }
 
   AnnotatedLine Line(ParsedLine);
   assert(Line.Last == LastToken);
 
   TokenAnnotator Annotator(Style, Keywords);
   Annotator.annotate(Line);
   Annotator.calculateFormattingInformation(Line);
 
-  const int Length = LastToken->TotalLength;
+  auto Length = LastToken->TotalLength;
+  if (OpeningBrace) {
+    assert(OpeningBrace != Tokens.front().Tok);
+    Length -= OpeningBrace->TokenText.size() + 1;
+  }
 
   Index = 0;
   for (auto &Token : Tokens) {
     const auto &SavedToken = SavedTokens[Index++];
     Token.Tok->copyFrom(*SavedToken.Tok);
     Token.Children = std::move(SavedToken.Children);
     delete SavedToken.Tok;
   }
 
   return Line.Level * Style.IndentWidth + Length <= ColumnLimit;
 }
 
 UnwrappedLineParser::IfStmtKind UnwrappedLineParser::parseBlock(
     bool MustBeDeclaration, unsigned AddLevels, bool MunchSemi, bool KeepBraces,
     bool UnindentWhitesmithsBraces, bool CanContainBracedList,
     TokenType NextLBracesType) {
   assert(FormatTok->isOneOf(tok::l_brace, TT_MacroBlockBegin) &&
          "'{' or macro block token expected");
   FormatToken *Tok = FormatTok;
+  const bool FollowedByComment = Tokens->peekNextToken()->is(tok::comment);
+  auto Index = CurrentLines->size();
   const bool MacroBlock = FormatTok->is(TT_MacroBlockBegin);
   FormatTok->setBlockKind(BK_Block);
 
   // For Whitesmiths mode, jump to the next level prior to skipping over the
   // braces.
   if (AddLevels > 0 && Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths)
     ++Line->Level;
 
UnwrappedLineParser::IfStmtKind UnwrappedLineParser::parseBlock(
 
   if (MacroBlock ? !FormatTok->is(TT_MacroBlockEnd)
                  : !FormatTok->is(tok::r_brace)) {
     Line->Level = InitialLevel;
     FormatTok->setBlockKind(BK_Block);
     return IfKind;
   }
 
-  if (SimpleBlock && !KeepBraces &&
-      Tok->isOneOf(TT_ControlStatementLBrace, TT_ElseLBrace)) {
+  if (SimpleBlock && !KeepBraces) {
+    assert(Tok->isOneOf(TT_ControlStatementLBrace, TT_ElseLBrace));
     assert(FormatTok->is(tok::r_brace));
     const FormatToken *Previous = Tokens->getPreviousToken();
     assert(Previous);
     if (Previous->isNot(tok::r_brace) || Previous->Optional) {
       assert(!CurrentLines->empty());
-      if (mightFitOnOneLine(CurrentLines->back())) {
+      const FormatToken *OpeningBrace = Tok;
+      if (!Tok->Previous) { // Wrapped l_brace.
+        if (FollowedByComment) {
+          KeepBraces = true;
+        } else {
+          assert(Index > 0);
+          --Index; // The line above the wrapped l_brace.
+          OpeningBrace = nullptr;
+        }
+      }
+      if (!KeepBraces && mightFitOnOneLine(CurrentLines->back()) &&
+          (Tok->is(TT_ElseLBrace) ||
+           mightFitOnOneLine((*CurrentLines)[Index], OpeningBrace))) {
         Tok->MatchingParen = FormatTok;
         FormatTok->MatchingParen = Tok;
       }
     }
   }
 
   size_t PPEndHash = computePPHash();
 

clang/unittests/Format/FormatTest.cpp

verifyFormat("if not consteval {\n"
                Style);
 
   verifyFormat("if !consteval {\n"
                "  g();\n"
                "}",
                Style);
 
   Style.ColumnLimit = 65;
-
   verifyFormat("if (condition) {\n"
                "  ff(Indices,\n"
                "     [&](unsigned LHSI, unsigned RHSI) { return true; });\n"
                "} else {\n"
                "  ff(Indices,\n"
                "     [&](unsigned LHSI, unsigned RHSI) { return true; });\n"
                "}",
                Style);
TEST_F(FormatTest, RemoveBraces) {
 
   verifyFormat("if (a)\n"
                "  b = c > 0 ? d : e;",
                "if (a) {\n"
                "  b = c > 0 ? d : e;\n"
                "}",
                Style);
 
-  Style.ColumnLimit = 0;
+  verifyFormat("if (-b >=\n"
+               "    c) { // Keep.\n"
+               "  foo();\n"
+               "} else {\n"
+               "  bar();\n"
+               "}",
+               "if (-b >= c) { // Keep.\n"
+               "  foo();\n"
+               "} else {\n"
+               "  bar();\n"
+               "}",
+               Style);
 
+  verifyFormat("if (a) /* Remove. */\n"
+               "  f();\n"
+               "else\n"
+               "  g();",
+               "if (a) <% /* Remove. */\n"
+               "  f();\n"
+               "%> else <%\n"
+               "  g();\n"
+               "%>",
+               Style);
+
+  verifyFormat("while (\n"
+               "    !i--) <% // Keep.\n"
+               "  foo();\n"
+               "%>",
+               "while (!i--) <% // Keep.\n"
+               "  foo();\n"
+               "%>",
+               Style);
+
+  verifyFormat("for (int &i : chars)\n"
+               "  ++i;",
+               "for (int &i :\n"
+               "     chars) {\n"
+               "  ++i;\n"
+               "}",
+               Style);
+
+  Style.ColumnLimit = 0;
   verifyFormat("if (a)\n"
                "  b234567890223456789032345678904234567890 = "
                "c234567890223456789032345678904234567890;",
                "if (a) {\n"
                "  b234567890223456789032345678904234567890 = "
                "c234567890223456789032345678904234567890;\n"
                "}",
                Style);
+
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterControlStatement = FormatStyle::BWACS_Always;
+  Style.BraceWrapping.BeforeElse = true;
+
+  Style.ColumnLimit = 65;
+
+  verifyFormat("if (condition)\n"
+               "{\n"
+               "  ff(Indices,\n"
+               "     [&](unsigned LHSI, unsigned RHSI) { return true; });\n"
+               "}\n"
+               "else\n"
+               "{\n"
+               "  ff(Indices,\n"
+               "     [&](unsigned LHSI, unsigned RHSI) { return true; });\n"
+               "}",
+               "if (condition) {\n"
+               "  ff(Indices,\n"
+               "     [&](unsigned LHSI, unsigned RHSI) { return true; });\n"
+               "} else {\n"
+               "  ff(Indices,\n"
+               "     [&](unsigned LHSI, unsigned RHSI) { return true; });\n"
+               "}",
+               Style);
+
+  verifyFormat("if (a)\n"
+               "{ //\n"
+               "  foo();\n"
+               "}",
+               "if (a) { //\n"
+               "  foo();\n"
+               "}",
+               Style);
+
+  Style.ColumnLimit = 20;
+
+  verifyFormat("int ab = [](int i) {\n"
+               "  if (i > 0)\n"
+               "  {\n"
+               "    i = 12345678 -\n"
+               "        i;\n"
+               "  }\n"
+               "  return i;\n"
+               "};",
+               "int ab = [](int i) {\n"
+               "  if (i > 0) {\n"
+               "    i = 12345678 -\n"
+               "        i;\n"
+               "  }\n"
+               "  return i;\n"
+               "};",
+               Style);
+
+  verifyFormat("if (a)\n"
+               "{\n"
+               "  b = c + // 1 -\n"
+               "      d;\n"
+               "}",
+               "if (a) {\n"
+               "  b = c + // 1 -\n"
+               "      d;\n"
+               "}",
+               Style);
+
+  verifyFormat("if (a)\n"
+               "{\n"
+               "  b = c >= 0 ? d\n"
+               "             : e;\n"
+               "}",
+               "if (a) {\n"
+               "  b = c >= 0 ? d : e;\n"
+               "}",
+               Style);
+
+  verifyFormat("if (a)\n"
+               "  b = c > 0 ? d : e;",
+               "if (a)\n"
+               "{\n"
+               "  b = c > 0 ? d : e;\n"
+               "}",
+               Style);
+
+  verifyFormat("if (foo + bar <=\n"
+               "    baz)\n"
+               "{\n"
+               "  func(arg1, arg2);\n"
+               "}",
+               "if (foo + bar <= baz) {\n"
+               "  func(arg1, arg2);\n"
+               "}",
+               Style);
+
+  verifyFormat("if (foo + bar < baz)\n"
+               "  func(arg1, arg2);\n"
+               "else\n"
+               "  func();",
+               "if (foo + bar < baz)\n"
+               "<%\n"
+               "  func(arg1, arg2);\n"
+               "%>\n"
+               "else\n"
+               "<%\n"
+               "  func();\n"
+               "%>",
+               Style);
+
+  verifyFormat("while (i--)\n"
+               "<% // Keep.\n"
+               "  foo();\n"
+               "%>",
+               "while (i--) <% // Keep.\n"
+               "  foo();\n"
+               "%>",
+               Style);
+
+  verifyFormat("for (int &i : chars)\n"
+               "  ++i;",
+               "for (int &i : chars)\n"
+               "{\n"
+               "  ++i;\n"
+               "}",
+               Style);
 }
 
 TEST_F(FormatTest, AlignAfterOpenBracketBlockIndent) {
   auto Style = getLLVMStyle();
 
   StringRef Short = "functionCall(paramA, paramB, paramC);\n"
                     "void functionDecl(int a, int b, int c);";