-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Comments
@llvm/issue-subscribers-clang-format |
same problem |
So this was done on purpose: 5a61139
We could add another option, but I think that will be a bit too hard to configure, opinions @mydeveloperday @owenca @djasper ? |
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 |
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. |
IMO if the comment after the
But clang-formant formats them differently:
I don't think we can avoid regressions if we decide to fix it. |
I already have an impression that |
No matter how good or robust clang-format is, it unfortunately can't read your mind |
Yes we have some crashes, but most of them are fixed very fast, so I'd call
A constructor call is also a function call, or not? Then the given example should also break
into
right? If there is no objection, I'll start trying to fix it for all cases. |
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.
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
A constructor call is a function call, in my opinion. |
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. |
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.
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 Applied to the given example:
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.
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. |
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});
So we should format everything as C1 and C4. Right? |
Yep! |
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.
to reproduce
.clang-format
main.cpp
:expected result
formatting is not changed
actual result
The space before
// comment
is removedversion
The text was updated successfully, but these errors were encountered: