Details
Diff Detail
Event Timeline
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?
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. |
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(
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? |
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. |
See D139257#3983332. If it gets reverted, we have to do something like D138263#3938593.
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 |
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.
clang/lib/Format/UnwrappedLineParser.cpp | ||
---|---|---|
2971–2972 | Nit. |
clang/lib/Format/UnwrappedLineParser.cpp | ||
---|---|---|
2971–2972 | Of course, this is a remainder of trying to get the MatchingParen, will fix before commit. |
Nit.