[clang] [clang-format] Allow open brace with trailing comment (no line break) (PR #89956)
owenca wrote: > @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/: > Also, if you have a [CODEOWNERS > file](https://github.blog/2017-07-06-introducing-code-owners/) in your > repository, a draft pull request will suppress notifications to those > reviewers until it is marked as ready for review. https://github.com/llvm/llvm-project/pull/89956 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Allow open brace with trailing comment (no line break) (PR #89956)
https://github.com/GertyP ready_for_review https://github.com/llvm/llvm-project/pull/89956 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Allow open brace with trailing comment (no line break) (PR #89956)
GertyP wrote: @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. https://github.com/llvm/llvm-project/pull/89956 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Allow open brace with trailing comment (no line break) (PR #89956)
owenca wrote: 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. `BreakBeforeTrailingComment`) instead of using `AlignTrailingComments`. https://github.com/llvm/llvm-project/pull/89956 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Allow open brace with trailing comment (no line break) (PR #89956)
owenca wrote: > > Ping > > Your PR is a draft, otherwise it would have been auto tagged and reviewers > would be informed. @GertyP can you remove the Draft status? https://github.com/llvm/llvm-project/pull/89956 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Allow open brace with trailing comment (no line break) (PR #89956)
GertyP wrote:
> 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 (`FormatTestComments.cpp`, ln 1437) doesn't
seem inappropriate to me. Is that the example you're referring to?
https://github.com/llvm/llvm-project/pull/89956
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Allow open brace with trailing comment (no line break) (PR #89956)
@@ -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 -
GertyP wrote:
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 '.'.
https://github.com/llvm/llvm-project/pull/89956
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Allow open brace with trailing comment (no line break) (PR #89956)
https://github.com/HazardyKnusperkeks requested changes to this pull request. The comments are not aligned in your example, that does not seem to be the right option to hook on. https://github.com/llvm/llvm-project/pull/89956 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Allow open brace with trailing comment (no line break) (PR #89956)
HazardyKnusperkeks wrote: > Ping Your PR is a draft, otherwise it would have been auto tagged and reviewers would be informed. https://github.com/llvm/llvm-project/pull/89956 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Allow open brace with trailing comment (no line break) (PR #89956)
@@ -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 -
mydeveloperday wrote:
extraneous - normally we end with a period (.)
https://github.com/llvm/llvm-project/pull/89956
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Allow open brace with trailing comment (no line break) (PR #89956)
mydeveloperday wrote: 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. https://github.com/llvm/llvm-project/pull/89956 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Allow open brace with trailing comment (no line break) (PR #89956)
mydeveloperday wrote: Sorry its the label that is important https://github.com/llvm/llvm-project/pull/89956 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Allow open brace with trailing comment (no line break) (PR #89956)
llvmbot wrote:
@llvm/pr-subscribers-clang-format
Author: None (GertyP)
Changes
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 `AlignTrailingComments` and
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 the `AlignTrailingComments :
Kind : Leave` control 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.
---
Full diff: https://github.com/llvm/llvm-project/pull/89956.diff
3 Files Affected:
- (modified) clang/lib/Format/ContinuationIndenter.cpp (+2-1)
- (modified) clang/lib/Format/TokenAnnotator.cpp (+24-4)
- (modified) clang/unittests/Format/FormatTestComments.cpp (+31)
``diff
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
ind
[clang] [clang-format] Allow open brace with trailing comment (no line break) (PR #89956)
https://github.com/GertyP edited https://github.com/llvm/llvm-project/pull/89956 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] clang-format: Allow open brace with trailing comment (no line break) (PR #89956)
GertyP wrote: Ping ... and tweaking the title formatting to use square braces in case it's important: Can someone tell me if it makes a difference to have `[clang-format] ...` in title over `clang-format: ...`? I haven't seen anything saying so in any readme but I could have missed something. https://github.com/llvm/llvm-project/pull/89956 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] clang-format: Allow open brace with trailing comment (no line break) (PR #89956)
GertyP wrote: Ping https://github.com/llvm/llvm-project/pull/89956 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] clang-format: Allow open brace with trailing comment (no line break) (PR #89956)
GertyP wrote: Ping https://github.com/llvm/llvm-project/pull/89956 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] clang-format: Allow open brace with trailing comment (no line break) (PR #89956)
GertyP wrote: Ping https://github.com/llvm/llvm-project/pull/89956 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] clang-format: Allow open brace with trailing comment (no line break) (PR #89956)
github-actions[bot] wrote: Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. 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 permissions for the repository. In which case you can instead tag reviewers by name in a comment by using `@` followed by their GitHub username. If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the [LLVM GitHub User Guide](https://llvm.org/docs/GitHub.html). You can also ask questions in a comment on this PR, on the [LLVM Discord](https://discord.com/invite/xS7Z362) or on the [forums](https://discourse.llvm.org/). https://github.com/llvm/llvm-project/pull/89956 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] clang-format: Allow open brace with trailing comment (no line break) (PR #89956)
https://github.com/GertyP created
https://github.com/llvm/llvm-project/pull/89956
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 `AlignTrailingComments` and
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 the `AlignTrailingComments :
Kind : Leave` control 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.
>From ec9d66f1f1e4ca71c3df097199551ba7caf4a9a6 Mon Sep 17 00:00:00 2001
From: Dan Hawson
Date: Mon, 22 Apr 2024 17:12:45 +0100
Subject: [PATCH] clang-format: Allow open brace with trailing comment (no line
break)
'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 #85083
---
clang/lib/Format/ContinuationIndenter.cpp | 3 +-
clang/lib/Format/TokenAnnotator.cpp | 28 ++---
clang/unittests/Format/FormatTestComments.cpp | 31 +++
3 files changed, 57 insertions(+), 5 deletions(-)
diff --git a/clang/lib/Format/ContinuationIndenter.cpp
b/clang/lib/Format/ContinuationIndenter.cpp
index ad0e2c3c620c32..dbc02aec0e0fc8 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 cdfb4256e41d93..2b13954df89ca9 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 AfterBracedIn
