[clang-format] Allow open brace with trailing comment (no line break) #89956
Conversation
'AlignTrailingComments: Kind: Leave' now avoids line breaks that were previously forced between an open brace and a trailing comment. Trailing comments after open braces can be desirable for nested braced blocks of a multi-dimensional array initializer. See llvm#85083
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
Ping |
1 similar comment
|
Ping |
|
Ping |
|
Ping |
GertyP
changed the title
|
@llvm/pr-subscribers-clang-format Author: None (GertyP) Changesclang-format always forcibly line-breaks between an initialiser open brace and a trailing comment, which separates the comment from the very thing it might directly relate to (the braced object). E.g. for things like nested braced blocks of a multi-dimensional array initialisation, keeping trailing comments next to their open braces can be a desirable formatting style. I.e. the comment is about the object associated with the brace, not the the first element in its initialisation. This relates to the issue mentioned in #85083 and does just about the minimum to fix only this case, through use of It could be argued that this 'keep trailing comment next to open brace' behaviour could also apply under other trailing comments 'kinds' or even that this may not perfectly fit within the scope of Full diff: https://github.com/llvm/llvm-project/pull/89956.diff 3 Files Affected:
diff --git a/clang/lib/Format/ContinuationIndenter.cpp b/clang/lib/Format/ContinuationIndenter.cpp
index ad0e2c3c620c3..dbc02aec0e0fc 100644
--- a/clang/lib/Format/ContinuationIndenter.cpp
+++ b/clang/lib/Format/ContinuationIndenter.cpp
@@ -831,7 +831,8 @@ void ContinuationIndenter::addTokenOnCurrentLine(LineState &State, bool DryRun,
Previous.isNot(TT_TableGenDAGArgOpenerToBreak) &&
!(Current.MacroParent && Previous.MacroParent) &&
(Current.isNot(TT_LineComment) ||
- Previous.isOneOf(BK_BracedInit, TT_VerilogMultiLineListLParen))) {
+ (Style.AlignTrailingComments.Kind != FormatStyle::TCAS_Leave &&
+ Previous.isOneOf(BK_BracedInit, TT_VerilogMultiLineListLParen)))) {
CurrentState.Indent = State.Column + Spaces;
CurrentState.IsAligned = true;
}
diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp
index cdfb4256e41d9..2b13954df89ca 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -5588,6 +5588,23 @@ bool TokenAnnotator::mustBreakBefore(const AnnotatedLine &Line,
}
if (BeforeClosingBrace && (BeforeClosingBrace->is(tok::comma) ||
BeforeClosingBrace->isTrailingComment())) {
+ // Except let's not always break after the open brace/parenthesis/array.
+ // A style option allowing keeping trailing comments together with the
+ // open token can be desirable. E.g -
+ // int a[2][2] = {
+ // { // [0][...]
+ // 0, // [0][0]
+ // 1, // [0][1]
+ // },
+ // { // [1][...]
+ // 2, // [1][0]
+ // 3, // [1][1]
+ // }
+ // };
+ if (Right.isTrailingComment() &&
+ Style.AlignTrailingComments.Kind == FormatStyle::TCAS_Leave) {
+ return false;
+ }
return true;
}
}
@@ -5997,10 +6014,13 @@ bool TokenAnnotator::canBreakBefore(const AnnotatedLine &Line,
if (Right.isTrailingComment()) {
// We rely on MustBreakBefore being set correctly here as we should not
// change the "binding" behavior of a comment.
- // The first comment in a braced lists is always interpreted as belonging to
- // the first list element. Otherwise, it should be placed outside of the
- // list.
- return Left.is(BK_BracedInit) ||
+ // The first comment in a braced lists is usually interpreted as belonging
+ // to the first list element, unless the style is to leave trailing comments
+ // alone. Otherwise, it should be placed outside of the list.
+ bool AfterBracedInitAndBrakeable =
+ Left.is(BK_BracedInit) &&
+ Style.AlignTrailingComments.Kind != FormatStyle::TCAS_Leave;
+ return AfterBracedInitAndBrakeable ||
(Left.is(TT_CtorInitializerColon) && Right.NewlinesBefore > 0 &&
Style.BreakConstructorInitializers == FormatStyle::BCIS_AfterColon);
}
diff --git a/clang/unittests/Format/FormatTestComments.cpp b/clang/unittests/Format/FormatTestComments.cpp
index d2baace6a7d80..04ca2f92a9579 100644
--- a/clang/unittests/Format/FormatTestComments.cpp
+++ b/clang/unittests/Format/FormatTestComments.cpp
@@ -1429,6 +1429,37 @@ TEST_F(FormatTestComments, CommentsInStaticInitializers) {
" 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // comment\n"
" 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // comment\n"
" 0x00, 0x00, 0x00, 0x00}; // comment");
+
+ // The usual 'open brace with trailing comment' behaviour is to forcibly
+ // break the trailing comment onto onto a new line -
+ FormatStyle Style = getLLVMStyle();
+ Style.AlignAfterOpenBracket = FormatStyle::BAS_BlockIndent;
+ StringRef Input = "int a[2][2] = {\n"
+ " { // a\n"
+ " 0, // x\n"
+ " 1,\n"
+ " },\n"
+ " {\n"
+ " 2,\n"
+ " 3, // y\n"
+ " }\n"
+ "};";
+ verifyFormat("int a[2][2] = {\n"
+ " {\n"
+ " // a\n"
+ " 0, // x\n"
+ " 1,\n"
+ " },\n"
+ " {\n"
+ " 2,\n"
+ " 3, // y\n"
+ " }\n"
+ "};",
+ Input, Style);
+ // But, especially for nested, multi-dimensional initialization, allowing
+ // open braces with trailing comments can be desirable -
+ Style.AlignTrailingComments.Kind = FormatStyle::TCAS_Leave;
+ verifyNoChange(Input, Style);
}
TEST_F(FormatTestComments, LineCommentsAfterRightBrace) {
|
|
Sorry its the label that is important |
mydeveloperday
requested review from
HazardyKnusperkeks,
mydeveloperday and
owenca
and removed request for
HazardyKnusperkeks
|
I don't see anything terrible, as long as all the existing test run ok, but @owenca and @HazardyKnusperkeks are the ones who tend to see much deeper things in the code changes than me.. lets let them have a look. |
| "};", | ||
| Input, Style); | ||
| // But, especially for nested, multi-dimensional initialization, allowing | ||
| // open braces with trailing comments can be desirable - |
There was a problem hiding this comment.
extraneous - normally we end with a period (.)
There was a problem hiding this comment.
Thanks.
"... -" is something I use to more explicitly link the comment to the lines that immediately follow (as opposed to anything before the comment), so it becomes more synonymous with "..., thus".
It hadn't occured to me that this could be misinterpreted or queried... until now, so I don't have a problem replacing the '-' with a '.'.
Your PR is a draft, otherwise it would have been auto tagged and reviewers would be informed. |
HazardyKnusperkeks
left a comment
HazardyKnusperkeks
left a comment
There was a problem hiding this comment.
The comments are not aligned in your example, that does not seem to be the right option to hook on.
I think I've misunderstood your point: A trailing comments alignment style of 'leave' (i.e. "don't mess around in any way with my trailing comments") seems the most suitable existing option to add this new functionality that extends the "leave alone" approach to now also avoiding otherwise forced line breaks between '{' and trailing comments too, so a test that preserves any unaligned trailing comments in the test input ( |
@GertyP can you remove the Draft status? |
|
IMO breaking before a trailing comment is a bug. Can we fix it without adding an option? If not, we should add one (e.g. |
|
@owenca Re. draft status: I thought a draft would be a better way (less presumptuous, more tentative suggestion) to present one particular solution, while acknowledging the possibility of alternatives. If that's not really the way a draft is used or if it's more of a problem (e.g. people simply filtering out drafts for whatever reason?) then I can remove the draftness; I just didn't want to presume this change is the way to go. |
My understanding is that draft pull requests are not ready for review, so I have always ignored them. From https://github.blog/2019-02-14-introducing-draft-pull-requests/:
|
5 participants
clang-format always forcibly line-breaks between an initialiser open brace and a trailing comment, which separates the comment from the very thing it might directly relate to (the braced object). E.g. for things like nested braced blocks of a multi-dimensional array initialisation, keeping trailing comments next to their open braces can be a desirable formatting style. I.e. the comment is about the object associated with the brace, not the the first element in its initialisation.
This relates to the issue mentioned in #85083 and does just about the minimum to fix only this case, through use of
AlignTrailingComments: Kind: Leave.It could be argued that this 'keep trailing comment next to open brace' behaviour could also apply under other trailing comments 'kinds' or even that this may not perfectly fit within the scope of
AlignTrailingCommentsand instead might warrant an entirely separate style option. I do wonder if this could be enough of an edge-case (i.e. no one's asked for this control so far and no other controls seem to explicitly define any initialiser-open-brace-with-trailing-comment behaviour) that this fairly minimal change is acceptable under the umbrella of theAlignTrailingComments : Kind : Leavecontrol or whether opinions are that this behaviour actually really belongs under some other control or even an entirely new option. If the latter, I think this would then require quite a few more changes, which is why I'm initially proposing this under the former, simpler approach but I'm interested in thoughts on this.