[clang-format] Fix SeparateDefinitionBlocks issues

Fixes https://github.com/llvm/llvm-project/issues/52976.

- Make no formatting for macros
- Attach comment with definition headers
- Make no change on use of empty lines at block start/end
- Fix misrecognition of keyword namespace

Differential Revision: https://reviews.llvm.org/D116663
Reviewed By: MyDeveloperDay, HazardyKnusperkeks, curdeius

clang/lib/Format/DefinitionBlockSeparator.cpp

@@ -31,15 +31,16 @@ std::pair<tooling::Replacements, unsigned> DefinitionBlockSeparator::analyze(
 
 void DefinitionBlockSeparator::separateBlocks(
     SmallVectorImpl<AnnotatedLine *> &Lines, tooling::Replacements &Result) {
+  const bool IsNeverStyle =
+      Style.SeparateDefinitionBlocks == FormatStyle::SDS_Never;
   auto LikelyDefinition = [this](const AnnotatedLine *Line) {
-    if (Line->MightBeFunctionDecl && Line->mightBeFunctionDefinition())
+    if ((Line->MightBeFunctionDecl && Line->mightBeFunctionDefinition()) ||
+        Line->startsWithNamespace())
       return true;
     FormatToken *CurrentToken = Line->First;
     while (CurrentToken) {
-      if (CurrentToken->isOneOf(tok::kw_class, tok::kw_struct,
+      if (CurrentToken->isOneOf(tok::kw_class, tok::kw_struct, tok::kw_enum) ||
-                                tok::kw_namespace, tok::kw_enum) ||
+          (Style.isJavaScript() && CurrentToken->TokenText == "function"))
-          (Style.Language == FormatStyle::LK_JavaScript &&
-           CurrentToken->TokenText == "function"))
         return true;
       CurrentToken = CurrentToken->Next;
     }
@@ -54,11 +55,14 @@ void DefinitionBlockSeparator::separateBlocks(
                 Env.getSourceManager().getBufferData(Env.getFileID()),
                 Style.UseCRLF)
           : Style.UseCRLF);
-  for (unsigned I = 0; I < Lines.size(); I++) {
+  for (unsigned I = 0; I < Lines.size(); ++I) {
     const auto &CurrentLine = Lines[I];
+    if (CurrentLine->InPPDirective)
+      continue;
     FormatToken *TargetToken = nullptr;
     AnnotatedLine *TargetLine;
     auto OpeningLineIndex = CurrentLine->MatchingOpeningBlockLineIndex;
+    AnnotatedLine *OpeningLine = nullptr;
     const auto InsertReplacement = [&](const int NewlineToInsert) {
       assert(TargetLine);
       assert(TargetToken);
@@ -72,9 +76,18 @@ void DefinitionBlockSeparator::separateBlocks(
                                     TargetToken->SpacesRequiredBefore - 1,
                                     TargetToken->StartsColumn);
     };
+    const auto IsPPConditional = [&](const size_t LineIndex) {
+      const auto &Line = Lines[LineIndex];
+      return Line->First->is(tok::hash) && Line->First->Next &&
+             Line->First->Next->isOneOf(tok::pp_if, tok::pp_ifdef, tok::pp_else,
+                                        tok::pp_ifndef, tok::pp_elifndef,
+                                        tok::pp_elifdef, tok::pp_elif,
+                                        tok::pp_endif);
+    };
     const auto FollowingOtherOpening = [&]() {
       return OpeningLineIndex == 0 ||
-             Lines[OpeningLineIndex - 1]->Last->opensScope();
+             Lines[OpeningLineIndex - 1]->Last->opensScope() ||
+             IsPPConditional(OpeningLineIndex - 1);
     };
     const auto HasEnumOnLine = [CurrentLine]() {
       FormatToken *CurrentToken = CurrentLine->First;
@@ -87,17 +100,29 @@ void DefinitionBlockSeparator::separateBlocks(
     };
 
     bool IsDefBlock = false;
+    const auto MayPrecedeDefinition = [&](const int Direction = -1) {
+      const size_t OperateIndex = OpeningLineIndex + Direction;
+      assert(OperateIndex < Lines.size());
+      const auto &OperateLine = Lines[OperateIndex];
+      return (Style.isCSharp() && OperateLine->First->is(TT_AttributeSquare)) ||
+             OperateLine->First->is(tok::comment);
+    };
 
     if (HasEnumOnLine()) {
       // We have no scope opening/closing information for enum.
       IsDefBlock = true;
       OpeningLineIndex = I;
-      TargetLine = CurrentLine;
+      while (OpeningLineIndex > 0 && MayPrecedeDefinition())
-      TargetToken = CurrentLine->First;
+        --OpeningLineIndex;
+      OpeningLine = Lines[OpeningLineIndex];
+      TargetLine = OpeningLine;
+      TargetToken = TargetLine->First;
       if (!FollowingOtherOpening())
         InsertReplacement(NewlineCount);
-      else
+      else if (IsNeverStyle)
         InsertReplacement(OpeningLineIndex != 0);
+      TargetLine = CurrentLine;
+      TargetToken = TargetLine->First;
       while (TargetToken && !TargetToken->is(tok::r_brace))
         TargetToken = TargetToken->Next;
       if (!TargetToken) {
@@ -108,40 +133,48 @@ void DefinitionBlockSeparator::separateBlocks(
       if (OpeningLineIndex > Lines.size())
         continue;
       // Handling the case that opening bracket has its own line.
-      OpeningLineIndex -= Lines[OpeningLineIndex]->First->TokenText == "{";
+      OpeningLineIndex -= Lines[OpeningLineIndex]->First->is(tok::l_brace);
-      AnnotatedLine *OpeningLine = Lines[OpeningLineIndex];
+      OpeningLine = Lines[OpeningLineIndex];
       // Closing a function definition.
       if (LikelyDefinition(OpeningLine)) {
         IsDefBlock = true;
-        if (OpeningLineIndex > 0) {
+        while (OpeningLineIndex > 0 && MayPrecedeDefinition())
-          OpeningLineIndex -=
+          --OpeningLineIndex;
-              Style.Language == FormatStyle::LK_CSharp &&
+        OpeningLine = Lines[OpeningLineIndex];
-              Lines[OpeningLineIndex - 1]->First->is(tok::l_square);
-          OpeningLine = Lines[OpeningLineIndex];
-        }
         TargetLine = OpeningLine;
         TargetToken = TargetLine->First;
         if (!FollowingOtherOpening()) {
           // Avoid duplicated replacement.
-          if (!TargetToken->opensScope())
+          if (TargetToken->isNot(tok::l_brace))
             InsertReplacement(NewlineCount);
-        } else
+        } else if (IsNeverStyle)
           InsertReplacement(OpeningLineIndex != 0);
       }
     }
 
     // Not the last token.
     if (IsDefBlock && I + 1 < Lines.size()) {
-      TargetLine = Lines[I + 1];
+      OpeningLineIndex = I + 1;
+      TargetLine = Lines[OpeningLineIndex];
       TargetToken = TargetLine->First;
 
       // No empty line for continuously closing scopes. The token will be
       // handled in another case if the line following is opening a
       // definition.
-      if (!TargetToken->closesScope()) {
+      if (!TargetToken->closesScope() && !IsPPConditional(OpeningLineIndex)) {
-        if (!LikelyDefinition(TargetLine))
+        // Check whether current line may be precedings of a definition line.
+        while (OpeningLineIndex + 1 < Lines.size() &&
+               MayPrecedeDefinition(/*Direction=*/0))
+          ++OpeningLineIndex;
+        TargetLine = Lines[OpeningLineIndex];
+        if (!LikelyDefinition(TargetLine)) {
+          TargetLine = Lines[I + 1];
+          TargetToken = TargetLine->First;
           InsertReplacement(NewlineCount);
-      } else {
+        }
+      } else if (IsNeverStyle) {
+        TargetLine = Lines[I + 1];
+        TargetToken = TargetLine->First;
         InsertReplacement(OpeningLineIndex != 0);
       }
     }

clang/unittests/Format/DefinitionBlockSeparatorTest.cpp

@@ -57,8 +57,9 @@ protected:
       InverseStyle.SeparateDefinitionBlocks = FormatStyle::SDS_Always;
     EXPECT_EQ(ExpectedCode.str(), separateDefinitionBlocks(ExpectedCode, Style))
         << "Expected code is not stable";
-    std::string InverseResult = separateDefinitionBlocks(Code, InverseStyle);
+    std::string InverseResult =
-    EXPECT_NE(Code.str(), InverseResult)
+        separateDefinitionBlocks(ExpectedCode, InverseStyle);
+    EXPECT_NE(ExpectedCode.str(), InverseResult)
         << "Inverse formatting makes no difference";
     std::string CodeToFormat =
         HasOriginalCode ? Code.str() : removeEmptyLines(Code);
@@ -129,49 +130,166 @@ TEST_F(DefinitionBlockSeparatorTest, Basic) {
                Style);
 }
 
+TEST_F(DefinitionBlockSeparatorTest, UntouchBlockStartStyle) {
+  // Returns a std::pair of two strings, with the first one for passing into
+  // Always test and the second one be the expected result of the first string.
+  auto MakeUntouchTest = [&](std::string BlockHeader, std::string BlockChanger,
+                             std::string BlockFooter, bool BlockEndNewLine) {
+    std::string CodePart1 = "enum Foo { FOO, BAR };\n"
+                            "\n"
+                            "/*\n"
+                            "test1\n"
+                            "test2\n"
+                            "*/\n"
+                            "int foo(int i, int j) {\n"
+                            "  int r = i + j;\n"
+                            "  return r;\n"
+                            "}\n";
+    std::string CodePart2 = "/* Comment block in one line*/\n"
+                            "enum Bar { FOOBAR, BARFOO };\n"
+                            "\n"
+                            "int bar3(int j, int k) {\n"
+                            "  // A comment\n"
+                            "  int r = j % k;\n"
+                            "  return r;\n"
+                            "}\n";
+    std::string CodePart3 = "int bar2(int j, int k) {\n"
+                            "  int r = j / k;\n"
+                            "  return r;\n"
+                            "}\n";
+    std::string ConcatAll = BlockHeader + CodePart1 + BlockChanger + CodePart2 +
+                            BlockFooter + (BlockEndNewLine ? "\n" : "") +
+                            CodePart3;
+    return std::make_pair(BlockHeader + removeEmptyLines(CodePart1) +
+                              BlockChanger + removeEmptyLines(CodePart2) +
+                              BlockFooter + removeEmptyLines(CodePart3),
+                          ConcatAll);
+  };
+
+  FormatStyle AlwaysStyle = getLLVMStyle();
+  AlwaysStyle.SeparateDefinitionBlocks = FormatStyle::SDS_Always;
+
+  FormatStyle NeverStyle = getLLVMStyle();
+  NeverStyle.SeparateDefinitionBlocks = FormatStyle::SDS_Never;
+
+  auto TestKit = MakeUntouchTest("#ifdef FOO\n\n", "\n#elifndef BAR\n\n",
+                                 "\n#endif\n\n", false);
+  verifyFormat(TestKit.first, AlwaysStyle, TestKit.second);
+  verifyFormat(TestKit.second, NeverStyle, removeEmptyLines(TestKit.second));
+
+  TestKit =
+      MakeUntouchTest("#ifdef FOO\n", "#elifndef BAR\n", "#endif\n", false);
+  verifyFormat(TestKit.first, AlwaysStyle, TestKit.second);
+  verifyFormat(TestKit.second, NeverStyle, removeEmptyLines(TestKit.second));
+
+  TestKit = MakeUntouchTest("namespace Ns {\n\n",
+                            "\n} // namespace Ns\n\n"
+                            "namespace {\n\n",
+                            "\n} // namespace\n", true);
+  verifyFormat(TestKit.first, AlwaysStyle, TestKit.second);
+  verifyFormat(TestKit.second, NeverStyle, removeEmptyLines(TestKit.second));
+
+  TestKit = MakeUntouchTest("namespace Ns {\n",
+                            "} // namespace Ns\n\n"
+                            "namespace {\n",
+                            "} // namespace\n", true);
+  verifyFormat(TestKit.first, AlwaysStyle, TestKit.second);
+  verifyFormat(TestKit.second, NeverStyle, removeEmptyLines(TestKit.second));
+}
+
 TEST_F(DefinitionBlockSeparatorTest, Always) {
   FormatStyle Style = getLLVMStyle();
   Style.SeparateDefinitionBlocks = FormatStyle::SDS_Always;
   std::string Prefix = "namespace {\n";
-  std::string Postfix = "enum Foo { FOO, BAR };\n"
+  std::string Infix = "\n"
-                        "\n"
+                      "// Enum test1\n"
-                        "int foo(int i, int j) {\n"
+                      "// Enum test2\n"
-                        "  int r = i + j;\n"
+                      "enum Foo { FOO, BAR };\n"
-                        "  return r;\n"
+                      "\n"
-                        "}\n"
+                      "/*\n"
+                      "test1\n"
+                      "test2\n"
+                      "*/\n"
+                      "int foo(int i, int j) {\n"
+                      "  int r = i + j;\n"
+                      "  return r;\n"
+                      "}\n"
+                      "\n"
+                      "// Foobar\n"
+                      "int i, j, k;\n"
+                      "\n"
+                      "// Comment for function\n"
+                      "// Comment line 2\n"
+                      "// Comment line 3\n"
+                      "int bar(int j, int k) {\n"
+                      "  int r = j * k;\n"
+                      "  return r;\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) {\n"
+                      "  // A comment\n"
+                      "  int r = j % k;\n"
+                      "  return r;\n"
+                      "}\n";
+  std::string Postfix = "\n"
+                        "} // namespace\n"
                         "\n"
+                        "namespace T {\n"
                         "int i, j, k;\n"
-                        "\n"
+                        "} // namespace T";
-                        "int bar(int j, int k) {\n"
+  verifyFormat(Prefix + removeEmptyLines(Infix) + removeEmptyLines(Postfix),
-                        "  int r = j * k;\n"
+               Style, Prefix + Infix + Postfix);
-                        "  return r;\n"
-                        "}\n"
-                        "\n"
-                        "enum Bar { FOOBAR, BARFOO };\n"
-                        "} // namespace";
-  verifyFormat(Prefix + "\n\n\n" + removeEmptyLines(Postfix), Style,
-               Prefix + Postfix);
 }
 
 TEST_F(DefinitionBlockSeparatorTest, Never) {
   FormatStyle Style = getLLVMStyle();
   Style.SeparateDefinitionBlocks = FormatStyle::SDS_Never;
   std::string Prefix = "namespace {\n";
-  std::string Postfix = "enum Foo { FOO, BAR };\n"
+  std::string Postfix = "// Enum test1\n"
+                        "// Enum test2\n"
+                        "enum Foo { FOO, BAR };\n"
                         "\n"
+                        "/*\n"
+                        "test1\n"
+                        "test2\n"
+                        "*/\n"
                         "int foo(int i, int j) {\n"
                         "  int r = i + j;\n"
                         "  return r;\n"
                         "}\n"
                         "\n"
+                        "// Foobar\n"
                         "int i, j, k;\n"
                         "\n"
+                        "// Comment for function\n"
+                        "// Comment line 2\n"
+                        "// Comment line 3\n"
                         "int bar(int j, int k) {\n"
                         "  int r = j * k;\n"
                         "  return r;\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) {\n"
+                        "  // A comment\n"
+                        "  int r = j % k;\n"
+                        "  return r;\n"
+                        "}\n"
                         "} // namespace";
   verifyFormat(Prefix + "\n\n\n" + Postfix, Style,
                Prefix + removeEmptyLines(Postfix));
@@ -181,31 +299,57 @@ TEST_F(DefinitionBlockSeparatorTest, OpeningBracketOwnsLine) {
   FormatStyle Style = getLLVMStyle();
   Style.BreakBeforeBraces = FormatStyle::BS_Allman;
   Style.SeparateDefinitionBlocks = FormatStyle::SDS_Always;
-  verifyFormat("enum Foo\n"
+  verifyFormat("namespace NS\n"
+               "{\n"
+               "// Enum test1\n"
+               "// Enum test2\n"
+               "enum Foo\n"
                "{\n"
                "  FOO,\n"
                "  BAR\n"
                "};\n"
                "\n"
+               "/*\n"
+               "test1\n"
+               "test2\n"
+               "*/\n"
                "int foo(int i, int j)\n"
                "{\n"
                "  int r = i + j;\n"
                "  return r;\n"
                "}\n"
                "\n"
+               "// Foobar\n"
                "int i, j, k;\n"
                "\n"
+               "// Comment for function\n"
+               "// Comment line 2\n"
+               "// Comment line 3\n"
                "int bar(int j, int k)\n"
                "{\n"
                "  int r = j * k;\n"
                "  return r;\n"
                "}\n"
                "\n"
+               "int bar2(int j, int k)\n"
+               "{\n"
+               "  int r = j / k;\n"
+               "  return r;\n"
+               "}\n"
+               "\n"
                "enum Bar\n"
                "{\n"
                "  FOOBAR,\n"
                "  BARFOO\n"
-               "};",
+               "};\n"
+               "\n"
+               "int bar3(int j, int k)\n"
+               "{\n"
+               "  // A comment\n"
+               "  int r = j % k;\n"
+               "  return r;\n"
+               "}\n"
+               "} // namespace NS",
                Style);
 }
 
@@ -215,21 +359,42 @@ TEST_F(DefinitionBlockSeparatorTest, Leave) {
   Style.MaxEmptyLinesToKeep = 3;
   std::string LeaveAs = "namespace {\n"
                         "\n"
+                        "// Enum test1\n"
+                        "// Enum test2\n"
                         "enum Foo { FOO, BAR };\n"
                         "\n\n\n"
+                        "/*\n"
+                        "test1\n"
+                        "test2\n"
+                        "*/\n"
                         "int foo(int i, int j) {\n"
                         "  int r = i + j;\n"
                         "  return r;\n"
                         "}\n"
                         "\n"
+                        "// Foobar\n"
                         "int i, j, k;\n"
                         "\n"
+                        "// Comment for function\n"
+                        "// Comment line 2\n"
+                        "// Comment line 3\n"
                         "int bar(int j, int k) {\n"
                         "  int r = j * k;\n"
                         "  return r;\n"
                         "}\n"
                         "\n"
+                        "int bar2(int j, int k) {\n"
+                        "  int r = j / k;\n"
+                        "  return r;\n"
+                        "}\n"
+                        "\n"
+                        "// Comment for inline enum\n"
                         "enum Bar { FOOBAR, BARFOO };\n"
+                        "int bar3(int j, int k) {\n"
+                        "  // A comment\n"
+                        "  int r = j % k;\n"
+                        "  return r;\n"
+                        "}\n"
                         "} // namespace";
   verifyFormat(LeaveAs, Style, LeaveAs);
 }
@@ -251,6 +416,7 @@ TEST_F(DefinitionBlockSeparatorTest, CSharp) {
                "internal static String toString() {\r\n"
                "}\r\n"
                "\r\n"
+               "// Comment for enum\r\n"
                "public enum var {\r\n"
                "  none,\r\n"
                "  @string,\r\n"
@@ -258,6 +424,7 @@ TEST_F(DefinitionBlockSeparatorTest, CSharp) {
                "  @enum\r\n"
                "}\r\n"
                "\r\n"
+               "// Test\r\n"
                "[STAThread]\r\n"
                "static void Main(string[] args) {\r\n"
                "  Console.WriteLine(\"HelloWorld\");\r\n"