[clang-format] Supress aligning of trailing namespace comments

[HazardyKnusperkeks]

Summary

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

[owenpan]

I suppose it's fairly easy to annotate the l_brace of a namespace? If so, then wouldn't it be better to do that?

[HazardyKnusperkeks]

In D138263#3936007, @owenpan wrote:

I suppose it's fairly easy to annotate the l_brace of a namespace? If so, then wouldn't it be better to do that?

But the r_brace has no MatchingParen, and I didn't want to go into that hole.

[HazardyKnusperkeks]

In D138263#3936536, @HazardyKnusperkeks wrote:

In D138263#3936007, @owenpan wrote:

I suppose it's fairly easy to annotate the l_brace of a namespace? If so, then wouldn't it be better to do that?

But the r_brace has no MatchingParen, and I didn't want to go into that hole.

One thing I thought about was adding a new BlockKind. But maybe I can look into the code that sets that and can add the MatchingParen.

[HazardyKnusperkeks]

Working on the annotation.

[owenpan]

In D138263#3937223, @HazardyKnusperkeks wrote:

In D138263#3936536, @HazardyKnusperkeks wrote:

In D138263#3936007, @owenpan wrote:

I suppose it's fairly easy to annotate the l_brace of a namespace? If so, then wouldn't it be better to do that?

But the r_brace has no MatchingParen, and I didn't want to go into that hole.

One thing I thought about was adding a new BlockKind. But maybe I can look into the code that sets that and can add the MatchingParen.

Instead, we can annotate the r_brace (in UnwrappedLineParser::parseBlock) after annotating the l_brace, which would enable us to keep the existing test case on line 4037 unchanged.

clang/unittests/Format/FormatTest.cpp

4020

This trailing comment was at the correct column and shouldn’t be aligned to the header guard comment below it.

[owenpan]

Something like the following:

diff --git a/clang/lib/Format/FormatToken.h b/clang/lib/Format/FormatToken.h
index 87515372046d..3dc5e411df55 100644
--- a/clang/lib/Format/FormatToken.h
+++ b/clang/lib/Format/FormatToken.h
@@ -98,6 +98,8 @@ namespace format {
   TYPE(MacroBlockBegin)                                                        \
   TYPE(MacroBlockEnd)                                                          \
   TYPE(ModulePartitionColon)                                                   \
+  TYPE(NamespaceLBrace)                                                        \
+  TYPE(NamespaceRBrace)                                                        \
   TYPE(NamespaceMacro)                                                         \
   TYPE(NonNullAssertion)                                                       \
   TYPE(NullCoalescingEqual)                                                    \
diff --git a/clang/lib/Format/UnwrappedLineParser.cpp b/clang/lib/Format/UnwrappedLineParser.cpp
index 18ec0844db3d..6eb1086015f0 100644
--- a/clang/lib/Format/UnwrappedLineParser.cpp
+++ b/clang/lib/Format/UnwrappedLineParser.cpp
@@ -920,6 +920,9 @@ FormatToken *UnwrappedLineParser::parseBlock(
     return IfLBrace;
   }
 
+  if (FormatTok->is(tok::r_brace) && Tok->is(TT_NamespaceLBrace))
+    FormatTok->setFinalizedType(TT_NamespaceRBrace);
+
   const bool IsFunctionRBrace =
       FormatTok->is(tok::r_brace) && Tok->is(TT_FunctionLBrace);
 
@@ -2961,6 +2964,7 @@ void UnwrappedLineParser::parseNamespace() {
     }
   }
   if (FormatTok->is(tok::l_brace)) {
+    FormatTok->setFinalizedType(TT_NamespaceLBrace);
     if (ShouldBreakBeforeBrace(Style, InitialToken))
       addUnwrappedLine();
 
diff --git a/clang/lib/Format/WhitespaceManager.cpp b/clang/lib/Format/WhitespaceManager.cpp
index 1f29f7ab917c..0867a8585b50 100644
--- a/clang/lib/Format/WhitespaceManager.cpp
+++ b/clang/lib/Format/WhitespaceManager.cpp
@@ -990,8 +990,7 @@ void WhitespaceManager::alignTrailingComments() {
     // If this comment follows an } in column 0, it probably documents the
     // closing of a namespace and we don't want to align it.
     bool FollowsRBraceInColumn0 = i > 0 && Changes[i].NewlinesBefore == 0 &&
-                                  Changes[i - 1].Tok->is(tok::r_brace) &&
-                                  Changes[i - 1].StartOfTokenColumn == 0;
+                                  Changes[i - 1].Tok->is(TT_NamespaceRBrace);
     bool WasAlignedWithStartOfNextLine = false;
     if (Changes[i].NewlinesBefore >= 1) { // A comment on its own line.
       unsigned CommentColumn = SourceMgr.getSpellingColumnNumber(

[HazardyKnusperkeks]

Now with the annotated brace.

[HazardyKnusperkeks]

In D138263#3938593, @owenpan wrote:

Something like the following:

diff --git a/clang/lib/Format/FormatToken.h b/clang/lib/Format/FormatToken.h
index 87515372046d..3dc5e411df55 100644
--- a/clang/lib/Format/FormatToken.h
+++ b/clang/lib/Format/FormatToken.h
@@ -98,6 +98,8 @@ namespace format {
   TYPE(MacroBlockBegin)                                                        \
   TYPE(MacroBlockEnd)                                                          \
   TYPE(ModulePartitionColon)                                                   \
+  TYPE(NamespaceLBrace)                                                        \
+  TYPE(NamespaceRBrace)                                                        \
   TYPE(NamespaceMacro)                                                         \
   TYPE(NonNullAssertion)                                                       \
   TYPE(NullCoalescingEqual)                                                    \
diff --git a/clang/lib/Format/UnwrappedLineParser.cpp b/clang/lib/Format/UnwrappedLineParser.cpp
index 18ec0844db3d..6eb1086015f0 100644
--- a/clang/lib/Format/UnwrappedLineParser.cpp
+++ b/clang/lib/Format/UnwrappedLineParser.cpp
@@ -920,6 +920,9 @@ FormatToken *UnwrappedLineParser::parseBlock(
     return IfLBrace;
   }
 
+  if (FormatTok->is(tok::r_brace) && Tok->is(TT_NamespaceLBrace))
+    FormatTok->setFinalizedType(TT_NamespaceRBrace);
+
   const bool IsFunctionRBrace =
       FormatTok->is(tok::r_brace) && Tok->is(TT_FunctionLBrace);
 
@@ -2961,6 +2964,7 @@ void UnwrappedLineParser::parseNamespace() {
     }
   }
   if (FormatTok->is(tok::l_brace)) {
+    FormatTok->setFinalizedType(TT_NamespaceLBrace);
     if (ShouldBreakBeforeBrace(Style, InitialToken))
       addUnwrappedLine();
 
diff --git a/clang/lib/Format/WhitespaceManager.cpp b/clang/lib/Format/WhitespaceManager.cpp
index 1f29f7ab917c..0867a8585b50 100644
--- a/clang/lib/Format/WhitespaceManager.cpp
+++ b/clang/lib/Format/WhitespaceManager.cpp
@@ -990,8 +990,7 @@ void WhitespaceManager::alignTrailingComments() {
     // If this comment follows an } in column 0, it probably documents the
     // closing of a namespace and we don't want to align it.
     bool FollowsRBraceInColumn0 = i > 0 && Changes[i].NewlinesBefore == 0 &&
-                                  Changes[i - 1].Tok->is(tok::r_brace) &&
-                                  Changes[i - 1].StartOfTokenColumn == 0;
+                                  Changes[i - 1].Tok->is(TT_NamespaceRBrace);
     bool WasAlignedWithStartOfNextLine = false;
     if (Changes[i].NewlinesBefore >= 1) { // A comment on its own line.
       unsigned CommentColumn = SourceMgr.getSpellingColumnNumber(

Actually read this only now. Basically doing the same thing.

[HazardyKnusperkeks]

Ping

[owenpan]

Can you rebase (preferably after landing D138402)?

clang/lib/Format/WhitespaceManager.cpp

992

Don't we still need to check Changes[i].NewlinesBefore == 0? How would this format the code below with FixNamespaceComments: false?

namespace A {
  ...
}
// comment

clang/unittests/Format/FormatTestComments.cpp

20–32

Was this already done in D138402?

3081

Should we add tests for FixNamespaceComments: false?

3124

Why would TCAS_Leave result in no space before the trailing comment?

[HazardyKnusperkeks]

clang/lib/Format/WhitespaceManager.cpp

992

Will check.

clang/unittests/Format/FormatTestComments.cpp

20–32

The first yes, the second no. I did just not update this diff, my commit is updated.

3081

Will do.

3124

It just did, didn't investigate or decide.
Most likely clang-format just adds it there and the space just comes from the other formatting, which is disabled with Leave. I'd say this is fine, Leave just means the coder decides on the position of the comments, and if that comment is added he can just move it around and clang-format will not touch it any further.

[owenpan]

In D138263#3975828, @owenpan wrote:

Can you rebase (preferably after landing D138402)?

See D139257#3983332. If it gets reverted, we have to do something like D138263#3938593.

[HazardyKnusperkeks]

I'll wait for the revert and then look how to go forth.

[owenpan]

clang/lib/Format/UnwrappedLineParser.cpp

2976–2977

To be future-proof, I'd set the type at the beginning of the block, right after line 2957.

clang/lib/Format/WhitespaceManager.cpp

991–993

To minimize the diff.

clang/unittests/Format/FormatTestComments.cpp

3086

3124

IMO there should be a single space or tab between the closing namespace brace and the namespace comment (existing or inserted) as AlignTrailingComments should not affect namespace comments. Anyway, we can fix it in another patch if necessary.

3182

And put the test case in a #if 0 block? I don't think it should be aligned with the namespace comment.

3185–3190

[owenpan]

clang/unittests/Format/FormatTestComments.cpp

3124

See D156065.

[MyDeveloperDay]

In D138263#3936536, @HazardyKnusperkeks wrote:

In D138263#3936007, @owenpan wrote:

I suppose it's fairly easy to annotate the l_brace of a namespace? If so, then wouldn't it be better to do that?

But the r_brace has no MatchingParen, and I didn't want to go into that hole.

How many times has that caught you out though?....I could have used this SO many times to constraint stuff... I wonder how painful a BraceMatchingPass would be, labelling all BraceTypes to Match each others.

[HazardyKnusperkeks]

In D138263#4535269, @MyDeveloperDay wrote:

In D138263#3936536, @HazardyKnusperkeks wrote:

In D138263#3936007, @owenpan wrote:

I suppose it's fairly easy to annotate the l_brace of a namespace? If so, then wouldn't it be better to do that?

But the r_brace has no MatchingParen, and I didn't want to go into that hole.

How many times has that caught you out though?....I could have used this SO many times to constraint stuff... I wonder how painful a BraceMatchingPass would be, labelling all BraceTypes to Match each others.

Yeah... see D139257 and D139760.

clang/unittests/Format/FormatTestComments.cpp

3182

I thought about it and came to the conclusion, that we (in this change) only want to suppress the alignment of namespace comments, not aligning other comments to the namespace comment.
But all is fine for me.

[owenpan]

clang/lib/Format/UnwrappedLineParser.cpp

2958–2959

Nit.

[HazardyKnusperkeks]

clang/lib/Format/UnwrappedLineParser.cpp

2958–2959

Of course, this is a remainder of trying to get the MatchingParen, will fix before commit.

clang/lib/Format/FormatToken.h

#define LIST_TOKEN_TYPES                                                       \
   TYPE(LambdaArrow)                                                            \
   TYPE(LambdaLBrace)                                                           \
   TYPE(LambdaLSquare)                                                          \
   TYPE(LeadingJavaAnnotation)                                                  \
   TYPE(LineComment)                                                            \
   TYPE(MacroBlockBegin)                                                        \
   TYPE(MacroBlockEnd)                                                          \
   TYPE(ModulePartitionColon)                                                   \
+  TYPE(NamespaceLBrace)                                                        \
   TYPE(NamespaceMacro)                                                         \
+  TYPE(NamespaceRBrace)                                                        \
   TYPE(NonNullAssertion)                                                       \
   TYPE(NullCoalescingEqual)                                                    \
   TYPE(NullCoalescingOperator)                                                 \
   TYPE(NullPropagatingOperator)                                                \
   TYPE(ObjCBlockLBrace)                                                        \
   TYPE(ObjCBlockLParen)                                                        \
   TYPE(ObjCDecl)                                                               \
   TYPE(ObjCForIn)                                                              \

clang/lib/Format/UnwrappedLineParser.cpp

FormatToken *UnwrappedLineParser::parseBlock(
 
   if (MacroBlock ? !FormatTok->is(TT_MacroBlockEnd)
                  : !FormatTok->is(tok::r_brace)) {
     Line->Level = InitialLevel;
     FormatTok->setBlockKind(BK_Block);
     return IfLBrace;
   }
 
+  if (FormatTok->is(tok::r_brace) && Tok->is(TT_NamespaceLBrace))
+    FormatTok->setFinalizedType(TT_NamespaceRBrace);
+
   const bool IsFunctionRBrace =
       FormatTok->is(tok::r_brace) && Tok->is(TT_FunctionLBrace);
 
   auto RemoveBraces = [=]() mutable {
     if (!SimpleBlock)
       return false;
     assert(Tok->isOneOf(TT_ControlStatementLBrace, TT_ElseLBrace));
     assert(FormatTok->is(tok::r_brace));
while (FormatTok->isOneOf(tok::identifier, tok::coloncolon, tok::kw_inline,
         parseSquare();
       else if (FormatTok->is(tok::l_paren))
         parseParens();
       else
         nextToken();
     }
   }
   if (FormatTok->is(tok::l_brace)) {
     if (ShouldBreakBeforeBrace(Style, InitialToken))
       addUnwrappedLine();
 
     unsigned AddLevels =
         Style.NamespaceIndentation == FormatStyle::NI_All ||
                 (Style.NamespaceIndentation == FormatStyle::NI_Inner &&
                  DeclarationScopeStack.size() > 1)
             ? 1u
             : 0u;
     bool ManageWhitesmithsBraces =
         AddLevels == 0u &&
         Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths;
 
     // If we're in Whitesmiths mode, indent the brace if we're not indenting
     // the whole block.
     if (ManageWhitesmithsBraces)
       ++Line->Level;
 
+    FormatToken *LBrace = FormatTok;
+    LBrace->setFinalizedType(TT_NamespaceLBrace);
+
     // Munch the semicolon after a namespace. This is more common than one would
     // think. Putting the semicolon into its own line is very ugly.
     parseBlock(/*MustBeDeclaration=*/true, AddLevels, /*MunchSemi=*/true,
                /*KeepBraces=*/true, /*IfKind=*/nullptr,
                ManageWhitesmithsBraces);
 
     addUnwrappedLine(AddLevels > 0 ? LineLevel::Remove : LineLevel::Keep);
 

clang/lib/Format/WhitespaceManager.cpp

for (unsigned i = 0, e = Changes.size(); i != e; ++i) {
 
     // If we don't create a replacement for this change, we have to consider
     // it to be immovable.
     if (!Changes[i].CreateReplacement)
       ChangeMaxColumn = ChangeMinColumn;
 
     if (i + 1 != e && Changes[i + 1].ContinuesPPDirective)
       ChangeMaxColumn -= 2;
-    // If this comment follows an } in column 0, it probably documents the
-    // closing of a namespace and we don't want to align it.
-    bool FollowsRBraceInColumn0 = i > 0 && Changes[i].NewlinesBefore == 0 &&
-                                  Changes[i - 1].Tok->is(tok::r_brace) &&
-                                  Changes[i - 1].StartOfTokenColumn == 0;
+
+    // We don't want to align namespace end comments.
+    bool DontAlignThisComment = i > 0 &&
+                                Changes[i - 1].Tok->is(TT_NamespaceRBrace) &&
+                                Changes[i].NewlinesBefore == 0;
     bool WasAlignedWithStartOfNextLine = false;
     if (Changes[i].NewlinesBefore >= 1) { // A comment on its own line.
       unsigned CommentColumn = SourceMgr.getSpellingColumnNumber(
           Changes[i].OriginalWhitespaceRange.getEnd());
       for (unsigned j = i + 1; j != e; ++j) {
         if (Changes[j].Tok->is(tok::comment))
           continue;
 
         unsigned NextColumn = SourceMgr.getSpellingColumnNumber(
             Changes[j].OriginalWhitespaceRange.getEnd());
         // The start of the next token was previously aligned with the
         // start of this comment.
         WasAlignedWithStartOfNextLine =
             CommentColumn == NextColumn ||
             CommentColumn == NextColumn + Style.IndentWidth;
         break;
       }
     }
     if (Style.AlignTrailingComments.Kind == FormatStyle::TCAS_Never ||
-        FollowsRBraceInColumn0) {
+        DontAlignThisComment) {
       alignTrailingComments(StartOfSequence, i, MinColumn);
       MinColumn = ChangeMinColumn;
       MaxColumn = ChangeMinColumn;
       StartOfSequence = i;
     } else if (BreakBeforeNext || Newlines > NewLineThreshold ||
                (ChangeMinColumn > MaxColumn || ChangeMaxColumn < MinColumn) ||
                // Break the comment sequence if the previous line did not end
                // in a trailing comment.

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, FormatsNamespaces) {
   verifyFormat("namespace {\n"
                "int SomeVariable = 0; // comment\n"
                "} // namespace",
                LLVMWithNoNamespaceFix);
   EXPECT_EQ("#ifndef HEADER_GUARD\n"
             "#define HEADER_GUARD\n"
             "namespace my_namespace {\n"
             "int i;\n"
             "} // my_namespace\n"
             "#endif // HEADER_GUARD",
             format("#ifndef HEADER_GUARD\n"
                    " #define HEADER_GUARD\n"
                    "   namespace my_namespace {\n"
                    "int i;\n"
                    "}    // my_namespace\n"
                    "#endif    // HEADER_GUARD",
                    LLVMWithNoNamespaceFix));
verifyFormat("namespace a\n"
                "        return;\n"
                "        }\n"
                "      };\n"
                "    struct B\n"
                "      {\n"
                "      int x;\n"
                "      };\n"
                "    } // namespace b\n"
-               "  }   // namespace a",
+               "  } // namespace a",
                WhitesmithsBraceStyle);
 
   verifyFormat("void f()\n"
                "  {\n"
                "  if (true)\n"
                "    {\n"
                "    a();\n"
                "    }\n"

clang/unittests/Format/FormatTestComments.cpp

 
 namespace clang {
 namespace format {
 namespace test {
 namespace {
 
 FormatStyle getGoogleStyle() { return getGoogleStyle(FormatStyle::LK_Cpp); }
 
 class FormatTestComments : public FormatTestBase {};
 
 //===----------------------------------------------------------------------===//
 // Tests for comments.
 //===----------------------------------------------------------------------===//
 
 TEST_F(FormatTestComments, UnderstandsSingleLineComments) {
   verifyFormat("//* */");
   verifyFormat("// line 1\n"
                "// line 2\n"
                "void f() {}\n");
 
   EXPECT_EQ("// comment\n", format("//comment\n"));
   EXPECT_EQ("// #comment\n", format("//#comment\n"));
 
   EXPECT_EQ("// comment\n"
             "// clang-format on\n",
             format("//comment\n"
                    "// clang-format on\n"));
 
   verifyFormat("void f() {\n"
EXPECT_EQ("int foo = 12345; // comment\n"
             "          // comment which is\n"
             "          // wrapped arround.\n",
             format("int foo = 12345; // comment\n"
                    "int bar = 1234;       // This is a very long comment\n"
                    "          // which is wrapped arround.\n",
                    Style));
 }
 
+TEST_F(FormatTestComments, DontAlignNamespaceComments) {
+  FormatStyle Style = getLLVMStyle();
+  Style.NamespaceIndentation = FormatStyle::NI_All;
+  Style.NamespaceMacros.push_back("TESTSUITE");
+  Style.ShortNamespaceLines = 0;
+
+  llvm::StringRef Input = "namespace A {\n"
+                          "  TESTSUITE(B) {\n"
+                          "    namespace C {\n"
+                          "      namespace D {} // namespace D\n"
+                          "      std::string Foo = Bar; // Comment\n"
+                          "      std::string BazString = Baz;   // C2\n"
+                          "    }          // namespace C\n"
+                          "  }\n"
+                          "} // NaMeSpAcE A";
+
+  EXPECT_TRUE(Style.FixNamespaceComments);
+  EXPECT_EQ(Style.AlignTrailingComments.Kind, FormatStyle::TCAS_Always);
+  verifyFormat("namespace A {\n"
+               "  TESTSUITE(B) {\n"
+               "    namespace C {\n"
+               "      namespace D {} // namespace D\n"
+               "      std::string Foo = Bar;       // Comment\n"
+               "      std::string BazString = Baz; // C2\n"
+               "    } // namespace C\n"
+               "  } // TESTSUITE(B)\n"
+               "} // NaMeSpAcE A",
+               Input, Style);
+
+  Style.AlignTrailingComments.Kind = FormatStyle::TCAS_Never;
+  verifyFormat("namespace A {\n"
+               "  TESTSUITE(B) {\n"
+               "    namespace C {\n"
+               "      namespace D {} // namespace D\n"
+               "      std::string Foo = Bar; // Comment\n"
+               "      std::string BazString = Baz; // C2\n"
+               "    } // namespace C\n"
+               "  } // TESTSUITE(B)\n"
+               "} // NaMeSpAcE A",
+               Input, Style);
+
+  Style.AlignTrailingComments.Kind = FormatStyle::TCAS_Leave;
+  verifyFormat("namespace A {\n"
+               "  TESTSUITE(B) {\n"
+               "    namespace C {\n"
+               "      namespace D {} // namespace D\n"
+               "      std::string Foo = Bar; // Comment\n"
+               "      std::string BazString = Baz;   // C2\n"
+               "    }          // namespace C\n"
+               "  }// TESTSUITE(B)\n"
+               "} // NaMeSpAcE A",
+               Input, Style);
+
+  Style.FixNamespaceComments = false;
+  Style.AlignTrailingComments.Kind = FormatStyle::TCAS_Always;
+  verifyFormat("namespace A {\n"
+               "  TESTSUITE(B) {\n"
+               "    namespace C {\n"
+               "      namespace D {} // namespace D\n"
+               "      std::string Foo = Bar;       // Comment\n"
+               "      std::string BazString = Baz; // C2\n"
+               "    } // namespace C\n"
+               "  }\n"
+               "} // NaMeSpAcE A",
+               Input, Style);
+
+  Style.AlignTrailingComments.Kind = FormatStyle::TCAS_Never;
+  verifyFormat("namespace A {\n"
+               "  TESTSUITE(B) {\n"
+               "    namespace C {\n"
+               "      namespace D {} // namespace D\n"
+               "      std::string Foo = Bar; // Comment\n"
+               "      std::string BazString = Baz; // C2\n"
+               "    } // namespace C\n"
+               "  }\n"
+               "} // NaMeSpAcE A",
+               Input, Style);
+
+  Style.AlignTrailingComments.Kind = FormatStyle::TCAS_Leave;
+  verifyFormat("namespace A {\n"
+               "  TESTSUITE(B) {\n"
+               "    namespace C {\n"
+               "      namespace D {} // namespace D\n"
+               "      std::string Foo = Bar; // Comment\n"
+               "      std::string BazString = Baz;   // C2\n"
+               "    }          // namespace C\n"
+               "  }\n"
+               "} // NaMeSpAcE A",
+               Input, Style);
+
+  Style.AlignTrailingComments.Kind = FormatStyle::TCAS_Always;
+  Style.FixNamespaceComments = true;
+  Input = "namespace A {\n"
+          "  int Foo;\n"
+          "  int Bar;\n"
+          "}\n"
+          "// Comment";
+
+  verifyFormat("namespace A {\n"
+               "  int Foo;\n"
+               "  int Bar;\n"
+               "} // namespace A\n"
+               "  // Comment", Input, Style);
+
+  Style.FixNamespaceComments = false;
+  verifyFormat("namespace A {\n"
+               "  int Foo;\n"
+               "  int Bar;\n"
+               "}\n"
+               "// Comment",
+               Input, Style);
+}
+
 TEST_F(FormatTestComments, AlignsBlockCommentDecorations) {
   EXPECT_EQ("/*\n"
             " */",
             format("/*\n"
                    "*/"));
   EXPECT_EQ("/*\n"
             " */",
             format("/*\n"