Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang-format]: no space before trailing comment #55487

Open
igagis opened this issue May 15, 2022 · 16 comments
Open

[clang-format]: no space before trailing comment #55487

igagis opened this issue May 15, 2022 · 16 comments
Assignees

Comments

@igagis
Copy link

igagis commented May 15, 2022

to reproduce

.clang-format

---
AlignAfterOpenBracket: BlockIndent
AlignArrayOfStructures: Right
BinPackArguments: 'false'
ColumnLimit: '120'
ContinuationIndentWidth: '1'
IndentWidth: '1'
Language: Cpp
SpacesBeforeTrailingComments: '1'
SpacesInLineCommentPrefix:
  Minimum: 1
  Maximum: -1
Standard: Latest
TabWidth: '1'
UseCRLF: 'false'
UseTab: ForContinuationAndIndentation

...

main.cpp:

type tree( //
	2,
	{ // comment
		tree(3, {78, 89, 96}),
		tree(4, {32, 64, 128}),
		tree(42, {98, 99, 100})
	}
);

expected result

formatting is not changed

actual result

type tree( //
	2,
	{// comment
		tree(3, {78, 89, 96}),
		tree(4, {32, 64, 128}),
		tree(42, {98, 99, 100})}
);

The space before // comment is removed

version

$ clang-format --version
Debian clang-format version 15.0.0-++20220513071846+693758b28295-1~exp1~20220513071937.249
@llvmbot
Copy link
Member

llvmbot commented May 15, 2022

@llvm/issue-subscribers-clang-format

@Eplankton
Copy link

same problem

@HazardyKnusperkeks HazardyKnusperkeks self-assigned this Nov 12, 2022
@HazardyKnusperkeks
Copy link
Contributor

So this was done on purpose: 5a61139
So how to fix it? If I remove that check and assignment, someone else will get it's code misformatted.
But if we replace the { and } with ty( and ) the space is kept (some newlines are lost), but that basically is against

Fundamentally, C++11 braced lists are formatted exactly like function calls would be formatted in their place.

We could add another option, but I think that will be a bit too hard to configure, opinions @mydeveloperday @owenca @djasper ?

@HazardyKnusperkeks
Copy link
Contributor

type tree( //
	2,
	{ // comment
		tree(3, {78, 89, 96}),
		tree(4, {32, 64, 128}),
		tree(42, {98, 99, 100})
	}
);

To keep the closing brace on a separate line, just add a trailing comma for the 3rd tree:

type tree( //
 	2,
 	{ // comment
 		tree(3, {78, 89, 96}),
 		tree(4, {32, 64, 128}),
 		tree(42, {98, 99, 100}),
 	}
 );

You may want to look into the InsertTrailingCommas option.

@igagis
Copy link
Author

igagis commented Nov 12, 2022

To keep the closing brace on a separate line, just add a trailing comma for the 3rd tree:

thanks for info, but this looks like a workaround. C++ doesn't require the last comma and clang-format should handle this case, as I see it.

@HazardyKnusperkeks
Copy link
Contributor

To keep the closing brace on a separate line, just add a trailing comma for the 3rd tree:

thanks for info, but this looks like a workaround. C++ doesn't require the last comma and clang-format should handle this case, as I see it.

It does handle it, just not the way you want it to. ;) Other people rely on this handling.

@owenca
Copy link
Contributor

owenca commented Nov 13, 2022

So this was done on purpose: 5a61139 So how to fix it? If I remove that check and assignment, someone else will get it's code misformatted. But if we replace the { and } with ty( and ) the space is kept (some newlines are lost), but that basically is against

Fundamentally, C++11 braced lists are formatted exactly like function calls would be formatted in their place.

We could add another option, but I think that will be a bit too hard to configure, opinions @mydeveloperday @owenca @djasper ?

IMO if the comment after the { or ( is actually meant for the first entry/argument below it, it should start on a new line instead:

$ cat test.cpp
void foo() {
  bar({
       // Comment 1
       "first entry",
       // Comment 2
       "second entry"});
  baz(
      // Comment 1
      "first argument",
      // Comment 2
      "second argument");
}

But clang-formant formats them differently:

$ clang-format test.cpp
void foo() {
  bar({// Comment 1
       "first entry",
       // Comment 2
       "second entry"});
  baz(
      // Comment 1
      "first argument",
      // Comment 2
      "second argument");
}

I don't think we can avoid regressions if we decide to fix it.

@igagis
Copy link
Author

igagis commented Nov 13, 2022

I don't think we can avoid regressions if we decide to fix it.

I already have an impression that clang-format is not a robust software, sadly, there's nothing better at the moment :(

@rymiel
Copy link
Member

rymiel commented Nov 13, 2022

No matter how good or robust clang-format is, it unfortunately can't read your mind

@HazardyKnusperkeks
Copy link
Contributor

I don't think we can avoid regressions if we decide to fix it.

I already have an impression that clang-format is not a robust software, sadly, there's nothing better at the moment :(

Yes we have some crashes, but most of them are fixed very fast, so I'd call clang-format robust.
Apart from that how does it play for you to insult the work some people did for free for you? I certainly hope the users of the code you write are a bit more well behaved.

IMO if the comment after the { or ( is actually meant for the first entry/argument below it, it should start on a new line instead:

$ cat test.cpp
void foo() {
  bar({
       // Comment 1
       "first entry",
       // Comment 2
       "second entry"});
  baz(
      // Comment 1
      "first argument",
      // Comment 2
      "second argument");
}

But clang-formant formats them differently:

$ clang-format test.cpp
void foo() {
  bar({// Comment 1
       "first entry",
       // Comment 2
       "second entry"});
  baz(
      // Comment 1
      "first argument",
      // Comment 2
      "second argument");
}

I don't think we can avoid regressions if we decide to fix it.

A constructor call is also a function call, or not? Then the given example should also break

type tree( //

into

type tree(
          //

right?

If there is no objection, I'll start trying to fix it for all cases.

@igagis
Copy link
Author

igagis commented Nov 20, 2022

Apart from that how does it play for you to insult the work some people did for free for you? I certainly hope the users of the code you write are a bit more well behaved.

I also produce opensource code and I had couple of times some not well behaved users, but they had no any argumentation to their complaints.
Sorry, I didn't mean to insult you, I just express my impression. I understand that people do the work for free, but come on, "we are not going to fix it because we are afraid to break something else" doesn't sound like an excuse in 2022, but instead it sounds like a fragile code with a lack of test coverage.

IMO if the comment after the { or ( is actually meant for the first entry/argument below it, it should start on a new line instead:

in my example, the comment meant to force clang-format to break the line, it is not a comment to any argument. I just gave a // comment word in the example in order to refer to the comment easier. In the use case it should have been just an empty comment // to break the line.
In my understanding there should never be a break before // comment inserted by clang-format, because single line comments are often given in the end of the line, for that exact line, not for the next line. And clang-format cannot guess what was the user intention in this case.

A constructor call is also a function call, or not?

A constructor call is a function call, in my opinion.

@rymiel
Copy link
Member

rymiel commented Nov 20, 2022

it sounds like a fragile code with a lack of test coverage

This isn't about lack of tests, there are plenty of tests. This is about people out there in the wild who might expect it to work the way it does currently. The original change was seemingly made for a reason, whatever the justification may be, so we can't know how many people would be affected that depend on the same justification.

@HazardyKnusperkeks
Copy link
Contributor

I understand that people do the work for free, but come on, "we are not going to fix it because we are afraid to break something else" doesn't sound like an excuse in 2022,

No one said that. I intend to fix it, but most likely not to your liking. What you want seems to be a new option.

but instead it sounds like a fragile code with a lack of test coverage.

That depends on how you measure test coverage. All new options (and I believe it was done the same in the past) are tested. But the possible input is nearly infinite and the combination of all options is myriad, if you want to test all of that you can now stop adding new stuff, because you never keep up with adding all the test combinations.

What I will do is making the code consistent. That means a // comment after an opening function call parenthesis or braces (in case of a constructor or an braced list) is meant to document the first parameter.

Applied to the given example:

type tree(
          //
	  2,
	  {
                // comment
		tree(3, {78, 89, 96}),
		tree(4, {32, 64, 128}),
		tree(42, {98, 99, 100})
	}
);

Output not guaranteed, because done by hand without checking all the implications.

After that "all" you need is a new option to disable the argument comment feature.

n my example, the comment meant to force clang-format to break the line, it is not a comment to any argument. I just gave a // comment word in the example in order to refer to the comment easier. In the use case it should have been just an empty comment // to break the line.
In my understanding there should never be a break before // comment inserted by clang-format, because single line comments are often given in the end of the line, for that exact line, not for the next line. And clang-format cannot guess what was the user intention in this case.

Initially I'd also thought so. But with the argument that a comment on this position is most likely for the first argument seems reasonable to me, because - apart from the forced line break - I can't think of a sensible use case for a comment there. And this decision was done on purpose, so there are most likely users (maybe even in LLVM) of said feature.

@owenca
Copy link
Contributor

owenca commented Nov 21, 2022

What I will do is making the code consistent. That means a // comment after an opening function call parenthesis or braces (in case of a constructor or an braced list) is meant to document the first parameter.

IMO a trailing comment (empty or not) belongs to the code before it. A comment about the code below it should starts on a new line. This means clang-format should not break before a trailing comment or change a non-trailing comment into a trailing one. We should fix it only if we don't have to change any existing unit tests, but I don't think we should add an option just for this.

@HazardyKnusperkeks
Copy link
Contributor

What I will do is making the code consistent. That means a // comment after an opening function call parenthesis or braces (in case of a constructor or an braced list) is meant to document the first parameter.

IMO a trailing comment (empty or not) belongs to the code before it. A comment about the code below it should starts on a new line. This means clang-format should not break before a trailing comment or change a non-trailing comment into a trailing one. We should fix it only if we don't have to change any existing unit tests, but I don't think we should add an option just for this.

Digging up the issue. How do we want to proceed? 5a61139 introduced that on purpose.

I think all of them should be formatted equally:

T foo( // C1
    arg);
T bar{// C2
      Arg};
T baz({// C3
       arg});
func( // C4
    arg);
func({// C5
      arg});

Cpp11BracedListStyle's documentation states:

Fundamentally, C++11 braced lists are formatted exactly like function calls would be formatted in their place. If the braced list follows a name (e.g. a type or variable name), clang-format formats as if the {} were the parentheses of a function call with that name. If there is no name, a zero-length name is assumed.

So we should format everything as C1 and C4. Right?

@owenca
Copy link
Contributor

owenca commented Nov 1, 2023

Yep!

HazardyKnusperkeks added a commit to HazardyKnusperkeks/llvm-project that referenced this issue Nov 10, 2023
Fixes http://llvm.org/PR55487 (llvm#55487)

The code did not match the documentation about Cpp11BracedListStyle.
Changed handling of comments after opening braces, which are supposedly
function call like to behave exactly like their parenthesis counter
part.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants