[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

4037

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

991

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

71–83

Was this already done in D138402?

3094

Should we add tests for FixNamespaceComments: false?

3137

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

[HazardyKnusperkeks]

clang/lib/Format/WhitespaceManager.cpp

991

Will check.

clang/unittests/Format/FormatTestComments.cpp

71–83

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

3094

Will do.

3137

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

2989–2990

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

clang/lib/Format/WhitespaceManager.cpp

990–992

To minimize the diff.

clang/unittests/Format/FormatTestComments.cpp

3099

3137

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.

3195

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

3198–3203

[owenpan]

clang/unittests/Format/FormatTestComments.cpp

3137

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

3195

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

2971–2972

Nit.

[HazardyKnusperkeks]

clang/lib/Format/UnwrappedLineParser.cpp

2971–2972

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

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);
+
     parseBlock(/*MustBeDeclaration=*/true, AddLevels, /*MunchSemi=*/true,
                /*KeepBraces=*/nullptr, /*IfKind=*/nullptr,
                ManageWhitesmithsBraces);
 
+    if (LBrace->MatchingParen)
+      LBrace->MatchingParen->setFinalizedType(TT_NamespaceRBrace);
+
     // Munch the semicolon after a namespace. This is more common than one would
     // think. Putting the semicolon into its own line is very ugly.
     if (FormatTok->is(tok::semi))
       nextToken();
 
     addUnwrappedLine(AddLevels > 0 ? LineLevel::Remove : LineLevel::Keep);
 
     if (ManageWhitesmithsBraces)

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);
     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

protected:
   }
 
   void verifyFormat(llvm::StringRef Code,
                     const FormatStyle &Style = getLLVMStyle()) {
     EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not stable";
     EXPECT_EQ(Code.str(), format(test::messUp(Code), Style));
   }
 
+  void verifyFormat(llvm::StringRef Expected, llvm::StringRef Code,
+                    const FormatStyle &Style = getLLVMStyle()) {
+    EXPECT_EQ(Expected.str(), format(Expected, Style))
+        << "Expected code is not stable";
+    EXPECT_EQ(Expected.str(), format(test::messUp(Code), Style));
+  }
+
+  void verifyFormatWithoutMessUp(llvm::StringRef Expected, llvm::StringRef Code,
+                                 const FormatStyle &Style = getLLVMStyle()) {
+    EXPECT_EQ(Expected.str(), format(Expected, Style))
+        << "Expected code is not stable";
+    EXPECT_EQ(Expected.str(), format(Code, Style));
+  }
+
   void verifyGoogleFormat(llvm::StringRef Code) {
     verifyFormat(Code, getGoogleStyle());
   }
 
   /// \brief Verify that clang-format does not crash on the given input.
   void verifyNoCrash(llvm::StringRef Code,
                      const FormatStyle &Style = getLLVMStyle()) {
     format(Code, Style, SC_DoNotCheck);
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"
+                          "    }          // namespace C\n"
+                          "  }\n"
+                          "} // NaMeSpAcE A";
+
+  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"
+               "    } // 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"
+               "    } // namespace C\n"
+               "  } // TESTSUITE(B)\n"
+               "} // NaMeSpAcE A",
+               Input, Style);
+
+  Style.AlignTrailingComments.Kind = FormatStyle::TCAS_Leave;
+  verifyFormatWithoutMessUp("namespace A {\n"
+                            "  TESTSUITE(B) {\n"
+                            "    namespace C {\n"
+                            "      namespace D {} // namespace D\n"
+                            "      std::string Foo = Bar; // Comment\n"
+                            "    }          // namespace C\n"
+                            "  }// TESTSUITE(B)\n"
+                            "} // NaMeSpAcE A",
+                            Input, Style);
+}
+
 TEST_F(FormatTestComments, AlignsBlockCommentDecorations) {
   EXPECT_EQ("/*\n"
             " */",
             format("/*\n"
                    "*/",
                    getLLVMStyle()));
   EXPECT_EQ("/*\n"
             " */",
EXPECT_EQ("/**\n"
             format("/**\n"
                    " * line */",
                    getLLVMStyle()));
   EXPECT_EQ("/**\n"
             " * line */",
             format("/**\n"
                    "  * line */",
                    getLLVMStyle()));
   EXPECT_EQ("/**\n"
             " * line */",
             format("/**\n"
                    "   * line */",
                    getLLVMStyle()));
   EXPECT_EQ("/**\n"
             " * line */",
             format("/**\n"
                    "    * line */",
                    getLLVMStyle()));
 
   // Align the end '*/' after a line.
   EXPECT_EQ("/*\n"
             " * line\n"
             " */",
             format("/*\n"
                    "* line\n"