This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Supress aligning of trailing namespace comments
ClosedPublic

Authored by HazardyKnusperkeks on Nov 17 2022, 11:02 PM.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2022, 11:02 PM
HazardyKnusperkeks requested review of this revision.Nov 17 2022, 11:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2022, 11:02 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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?

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.

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 planned changes to this revision.Nov 18 2022, 12:02 PM

Working on the annotation.

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.

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 updated this revision to Diff 476722.EditedNov 19 2022, 8:10 PM

Now with the annotated brace.

HazardyKnusperkeks marked an inline comment as done.Nov 19 2022, 8:11 PM

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.

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?

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.

Can you rebase (preferably after landing D138402)?

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

HazardyKnusperkeks planned changes to this revision.Dec 9 2022, 11:49 AM

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

HazardyKnusperkeks marked 3 inline comments as done.
owenpan accepted this revision.Jul 21 2023, 6:18 PM
owenpan added inline comments.
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
This revision is now accepted and ready to land.Jul 21 2023, 6:18 PM
owenpan added inline comments.Jul 23 2023, 1:58 PM
clang/unittests/Format/FormatTestComments.cpp
3124

See D156065.

HazardyKnusperkeks marked 8 inline comments as done.

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.

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 accepted this revision.Jul 28 2023, 2:27 AM
owenpan added inline comments.
clang/lib/Format/UnwrappedLineParser.cpp
2958–2959

Nit.

HazardyKnusperkeks marked an inline comment as done.Jul 28 2023, 12:47 PM
HazardyKnusperkeks added inline comments.
clang/lib/Format/UnwrappedLineParser.cpp
2958–2959

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

This revision was landed with ongoing or failed builds.Aug 2 2023, 2:50 AM
This revision was automatically updated to reflect the committed changes.
HazardyKnusperkeks marked an inline comment as done.