[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

305

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.

55

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

51–64

Please use a for loop instead:

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

121–130

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

305

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

305

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;
+    else if (Tok->isOneOf(tok::r_brace, tok::r_paren, tok::r_square))
+      return -1;
+    else
+      return 0;
+  };
+  auto LikelyDefinition = [&](const AnnotatedLine *Line,
     if ((Line->MightBeFunctionDecl && Line->mightBeFunctionDefinition()) ||
         Line->startsWithNamespace())
       return true;
     FormatToken *CurrentToken = Line->First;
+    int BracketLevel = 0;
     while (CurrentToken) {
-      if (CurrentToken->isOneOf(tok::kw_class, tok::kw_struct) ||
-          (Style.isJavaScript() && CurrentToken->is(ExtraKeywords.kw_function)))
+      if (BracketLevel == 0) {
+        if ((CurrentToken->isOneOf(tok::kw_class, tok::kw_struct) ||
+             (Style.isJavaScript() &&
+              CurrentToken->is(ExtraKeywords.kw_function))))
           return true;
         if (!ExcludeEnum && CurrentToken->is(tok::kw_enum))
           return true;
+      }
+      BracketLevel += GetBracketLevelChange(CurrentToken);
       CurrentToken = CurrentToken->Next;
     }
     return false;
   };
   unsigned NewlineCount =
       (Style.SeparateDefinitionBlocks == FormatStyle::SDS_Always ? 1 : 0) + 1;
   WhitespaceManager Whitespaces(
       Env.getSourceManager(), Style,
       Style.DeriveLineEnding
           ? WhitespaceManager::inputUsesCRLF(
for (unsigned I = 0; I < Lines.size(); ++I) {
     const auto FollowingOtherOpening = [&]() {
       return OpeningLineIndex == 0 ||
              Lines[OpeningLineIndex - 1]->Last->opensScope() ||
              IsPPConditional(OpeningLineIndex - 1);
     };
     const auto HasEnumOnLine = [&]() {
       FormatToken *CurrentToken = CurrentLine->First;
       bool FoundEnumKeyword = false;
+      int BracketLevel = 0;
       while (CurrentToken) {
+        if (BracketLevel == 0) {
           if (CurrentToken->is(tok::kw_enum))
             FoundEnumKeyword = true;
           else if (FoundEnumKeyword && CurrentToken->is(tok::l_brace))
             return true;
+        }
+        BracketLevel += GetBracketLevelChange(CurrentToken);
         CurrentToken = CurrentToken->Next;
       }
       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

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"
+                      "  if (struct S = getS()) {\n"
+                      "    // if condition\n"
+                      "  }\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"
std::string Postfix = "// Enum test1\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"
+                        "  if (struct S = getS()) {\n"
+                        "    // if condition\n"
+                        "  }\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"
verifyFormat("namespace NS\n"
                "    int r = j * k;\n"
                "    return r;\n"
                "  }\n"
                "}\n"
                "\n"
                "int bar2(int j, int k)\n"
                "{\n"
                "  int r = j / k;\n"
+               "  if (struct S = getS())\n"
+               "  {\n"
+               "    // if condition\n"
+               "  }\n"
                "  return r;\n"
                "}\n"
                "\n"
                "enum Bar\n"
                "{\n"
                "  FOOBAR,\n"
                "  BARFOO\n"
                "};\n"
std::string LeaveAs = "namespace {\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"
+                        "  if (struct S = getS()) {\n"
+                        "    // if condition\n"
+                        "  }\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"