[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.