[clang-format] Fix DefinitionBlockSeparator extra empty lines

[ksyx]

Summary

• Add or remove empty lines surrounding unions
• Fixes https://github.com/llvm/llvm-project/issues/53229, in which keywords like class and struct in a line ending with left brace or whose next line is left brace only, will be falsely recognized as definition line, causing extra empty lines inserted surrounding blocks with no need to be formatted.

[curdeius]

Looks ok but please rework the tests to keep previous cases untouched.

clang/unittests/Format/DefinitionBlockSeparatorTest.cpp

314

I'd rather see this added in another function e.g. bar3, because otherwise you don't test what was tested before.

[HazardyKnusperkeks]

clang/lib/Format/DefinitionBlockSeparator.cpp

39

I would prefer a switch over the tokens.

But definitely no else after return.

50

What about union?

[ksyx]

• Add or remove empty lines surrounding unions.
• Move new test location.
• Remove else immediately above return.

[HazardyKnusperkeks]

clang/lib/Format/DefinitionBlockSeparator.cpp

41

This is also an else after return. ;)

[ksyx]

• Remove else after return

[owenpan]

clang/lib/Format/DefinitionBlockSeparator.cpp

49–63

Please use a for loop instead:

for (const FormatToken *CurrentToken = Line->First; CurrentToken;
     CurrentToken = CurrentToken->Next) {
  ...;
}

117–128

Use a for loop with the const FormatToken *CurrentToken loop variable similar to the above.

[ksyx]

Use for loop to go through tokens in a line.

[curdeius]

LGTM.

clang/unittests/Format/DefinitionBlockSeparatorTest.cpp

314

When I wrote this comment I thought that bar3 didn't exist... But well, I think I'll create a patch to clean up these tests a bit anyway.

[ksyx]

clang/unittests/Format/DefinitionBlockSeparatorTest.cpp

314

Thank you very much for this!

clang/lib/Format/DefinitionBlockSeparator.cpp

 }
 
 void DefinitionBlockSeparator::separateBlocks(
     SmallVectorImpl<AnnotatedLine *> &Lines, tooling::Replacements &Result,
     FormatTokenLexer &Tokens) {
   const bool IsNeverStyle =
       Style.SeparateDefinitionBlocks == FormatStyle::SDS_Never;
   const AdditionalKeywords &ExtraKeywords = Tokens.getKeywords();
-  auto LikelyDefinition = [this, ExtraKeywords](const AnnotatedLine *Line,
-                                                bool ExcludeEnum = false) {
+  auto GetBracketLevelChange = [](const FormatToken *Tok) {
+    if (Tok->isOneOf(tok::l_brace, tok::l_paren, tok::l_square))
+      return 1;
+    if (Tok->isOneOf(tok::r_brace, tok::r_paren, tok::r_square))
+      return -1;
+    return 0;
+  };
+  auto LikelyDefinition = [&](const AnnotatedLine *Line,
     if ((Line->MightBeFunctionDecl && Line->mightBeFunctionDefinition()) ||
         Line->startsWithNamespace())
       return true;
-    FormatToken *CurrentToken = Line->First;
-    while (CurrentToken) {
-      if (CurrentToken->isOneOf(tok::kw_class, tok::kw_struct) ||
-          (Style.isJavaScript() && CurrentToken->is(ExtraKeywords.kw_function)))
+    int BracketLevel = 0;
+    for (const FormatToken *CurrentToken = Line->First; CurrentToken;
+         CurrentToken = CurrentToken->Next) {
+      if (BracketLevel == 0) {
+        if ((CurrentToken->isOneOf(tok::kw_class, tok::kw_struct,
+                                   tok::kw_union) ||
+             (Style.isJavaScript() &&
+              CurrentToken->is(ExtraKeywords.kw_function))))
           return true;
         if (!ExcludeEnum && CurrentToken->is(tok::kw_enum))
           return true;
-      CurrentToken = CurrentToken->Next;
+      }
+      BracketLevel += GetBracketLevelChange(CurrentToken);
     }
     return false;
   };
   unsigned NewlineCount =
       (Style.SeparateDefinitionBlocks == FormatStyle::SDS_Always ? 1 : 0) + 1;
   WhitespaceManager Whitespaces(
       Env.getSourceManager(), Style,
       Style.DeriveLineEnding
           ? WhitespaceManager::inputUsesCRLF(
const auto IsPPConditional = [&](const size_t LineIndex) {
                                         tok::pp_endif);
     };
     const auto FollowingOtherOpening = [&]() {
       return OpeningLineIndex == 0 ||
              Lines[OpeningLineIndex - 1]->Last->opensScope() ||
              IsPPConditional(OpeningLineIndex - 1);
     };
     const auto HasEnumOnLine = [&]() {
-      FormatToken *CurrentToken = CurrentLine->First;
       bool FoundEnumKeyword = false;
-      while (CurrentToken) {
+      int BracketLevel = 0;
+      for (const FormatToken *CurrentToken = CurrentLine->First; CurrentToken;
+           CurrentToken = CurrentToken->Next) {
+        if (BracketLevel == 0) {
           if (CurrentToken->is(tok::kw_enum))
             FoundEnumKeyword = true;
           else if (FoundEnumKeyword && CurrentToken->is(tok::l_brace))
             return true;
-        CurrentToken = CurrentToken->Next;
+        }
+        BracketLevel += GetBracketLevelChange(CurrentToken);
       }
       return FoundEnumKeyword && I + 1 < Lines.size() &&
              Lines[I + 1]->First->is(tok::l_brace);
     };
 
     bool IsDefBlock = false;
     const auto MayPrecedeDefinition = [&](const int Direction = -1) {
       assert(Direction >= -1);
       assert(Direction <= 1);

clang/unittests/Format/DefinitionBlockSeparatorTest.cpp

verifyFormat("struct foo {\n"
                "  int i, j;\n"
                "};\n"
                "\n"
                "struct bar {\n"
                "  int j, k;\n"
                "};",
                Style);
 
+  verifyFormat("union foo {\n"
+               "  int i, j;\n"
+               "};\n"
+               "\n"
+               "union bar {\n"
+               "  int j, k;\n"
+               "};",
+               Style);
+
   verifyFormat("class foo {\n"
                "  int i, j;\n"
                "};\n"
                "\n"
                "class bar {\n"
                "  int j, k;\n"
                "};",
                Style);
std::string Infix = "\n"
                       "  {\n"
                       "    int r = j * k;\n"
                       "    return r;\n"
                       "  }\n"
                       "}\n"
                       "\n"
                       "int bar2(int j, int k) {\n"
                       "  int r = j / k;\n"
                       "  return r;\n"
                       "}\n"
                       "\n"
                       "/* Comment block in one line*/\n"
                       "enum Bar { FOOBAR, BARFOO };\n"
                       "\n"
                       "int bar3(int j, int k, const enum Bar b) {\n"
                       "  // A comment\n"
                       "  int r = j % k;\n"
+                      "  if (struct S = getS()) {\n"
+                      "    // if condition\n"
+                      "  }\n"
                       "  return r;\n"
                       "}\n";
   std::string Postfix = "\n"
                         "} // namespace\n"
                         "\n"
                         "namespace T {\n"
                         "int i, j, k;\n"
                         "} // namespace T";
std::string Postfix = "// Enum test1\n"
                         "}\n"
                         "\n"
                         "/* Comment block in one line*/\n"
                         "enum Bar { FOOBAR, BARFOO };\n"
                         "\n"
                         "int bar3(int j, int k, const enum Bar b) {\n"
                         "  // A comment\n"
                         "  int r = j % k;\n"
+                        "  if (struct S = getS()) {\n"
+                        "    // if condition\n"
+                        "  }\n"
                         "  return r;\n"
                         "}\n"
                         "} // namespace";
   verifyFormat(Prefix + "\n\n\n" + Postfix, Style,
                Prefix + removeEmptyLines(Postfix));
 }
 
 TEST_F(DefinitionBlockSeparatorTest, OpeningBracketOwnsLine) {
verifyFormat("namespace NS\n"
                "  FOOBAR,\n"
                "  BARFOO\n"
                "};\n"
                "\n"
                "int bar3(int j, int k, const enum Bar b)\n"
                "{\n"
                "  // A comment\n"
                "  int r = j % k;\n"
+               "  if (struct S = getS())\n"
+               "  {\n"
+               "    // if condition\n"
+               "  }\n"
                "  return r;\n"
                "}\n"
                "} // namespace NS",
                Style);
 }
 
 TEST_F(DefinitionBlockSeparatorTest, Leave) {
   FormatStyle Style = getLLVMStyle();
std::string LeaveAs = "namespace {\n"
                         "  return r;\n"
                         "}\n"
                         "\n"
                         "// Comment for inline enum\n"
                         "enum Bar { FOOBAR, BARFOO };\n"
                         "int bar3(int j, int k, const enum Bar b) {\n"
                         "  // A comment\n"
                         "  int r = j % k;\n"
+                        "  if (struct S = getS()) {\n"
+                        "    // if condition\n"
+                        "  }\n"
                         "  return r;\n"
                         "}\n"
                         "} // namespace";
   verifyFormat(LeaveAs, Style, LeaveAs);
 }
 
 TEST_F(DefinitionBlockSeparatorTest, CSharp) {
   FormatStyle Style = getLLVMStyle(FormatStyle::LK_CSharp);