Move C++ method definition 'static, virtual, etc' comments to be on new line
Categories
(Developer Infrastructure :: Lint and Formatting, enhancement, P3)
Tracking
(firefox67 fixed)
Tracking | Status | |
---|---|---|
firefox67 | --- | fixed |
People
(Reporter: rhunt, Assigned: rhunt)
References
(Depends on 1 open bug)
Details
Attachments
(27 files, 1 obsolete file)
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
Bug 1523969 part 25 - Move method definition inline comments to new line in 'uriloader/'. r?bzbarsky
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details |
Not sure if this is the right component.
This would reformat
/* static */ void Foo::Bar() {
...
}
To
// static
void Foo::Bar() {
...
}
See [1] for context.
[1] https://groups.google.com/d/msg/mozilla.dev.platform/6_reFXkmZIA/pM_TGZHqCwAJ
Comment 1•6 years ago
|
||
Thanks for filing the bug.
Just repeating my claim from the newsgroup thread here, because this is a better place to discuss and I'm not sure the claim was understood, that there is no gain in changing from /* */
to //
comments.
I acknowledge that clang-format treats the two differently in this situation, but this is not something that anyone would do with //
comments, and so the difference is of no advantage.
/* static */ void
Foo::Bar() {
// static void
Foo::Bar() {
In other situations, /* static */
behaves better with clang-format, and IMO is more clearly distinct from other comments.
Running clang-format on
// This comment is toooooooooooooooooooooooooooooooooooooooooooooooooooooooooo long.
/* static */
// This comment is toooooooooooooooooooooooooooooooooooooooooooooooooooooooooo long.
// static
produces
// This comment is toooooooooooooooooooooooooooooooooooooooooooooooooooooooooo
// long.
/* static */
// This comment is toooooooooooooooooooooooooooooooooooooooooooooooooooooooooo
// long. static
Adding blank lines is not desirable for the same reasons as are behind the principle in the style guide: "don't use blank lines when you don't have to."
Comment 2•6 years ago
|
||
I like Gerald's suggestion for a macro, and I'd be happy with that on a different line, even though this is all inconsistent with the style guide:
Attributes, and macros that expand to attributes, appear at the very beginning of the function declaration or definition, before the return type:
MUST_USE_RESULT bool IsOK();
Using a macro for this information feels like working against the tool, to me.
Writing static/etc. in comments is not an unheard of convention. Has anyone asked upstream whether they would accept an option to preserve such comments?
Linking with bug 1523793 for the macro suggestion, which goes in a different direction from the issue here: here is more about dealing with the automated formatting, there is more about statically-checked correctness of these annotations.
So they both could be discussed separately -- unless/until we find that one makes the other moot.
Comment 5•6 years ago
|
||
(In reply to Karl Tomlinson (:karlt) from comment #1)
Adding blank lines is not desirable for the same reasons as are behind the principle in the style guide: "don't use blank lines when you don't have to."
Well, to me it seems like the example in your comment shows that in this case we have to add a blank line.
If I understand your objection, it's not that you're saying adding a blank line wouldn't solve the problem you're pointing out, it's that you're saying you don't like that solution, and you prefer instead to keep the current state of the world, which is keep the C-style /* static */ comments on the first line of function definitions. Do I understand your position correctly?
Comment 6•6 years ago
|
||
(In reply to :Ehsan Akhgari from comment #5)
Well, to me it seems like the example in your comment shows that in this case we have to add a blank line.
The example was intended to demonstrate that clang-format folds //
comments together into one comment, but does not do the same for /* */
comments.
I can understand the benefit of a blank line when only //
comments are used, but the blank line is not necessary with /* static */
.
If I understand your objection, it's not that you're saying adding a blank line wouldn't solve the problem you're pointing out, it's that you're saying you don't like that solution, and you prefer instead to keep the current state of the world, which is keep the C-style /* static */ comments on the first line of function definitions. Do I understand your position correctly?
Partially.
You are correct in interpreting that I don't like having a blank line, thanks. i.e.
// This comment is toooooooooooooooooooooooooooooooooooooooooooooooooooooooooo
// long.
// static
void Foo::Bar() {
I also don't like the current state of the world with /* static */ on the same line as the function name. This is shifting the function name to the right, which can lead to truncation, but in some tools the opening comment means that some tools ignore the line altogether and show a previous function name. i.e.
// This comment is toooooooooooooooooooooooooooooooooooooooooooooooooooooooooo
// long.
/* static */ void Foo::Bar() {
IMO this is a better solution to either of the above.
// This comment is toooooooooooooooooooooooooooooooooooooooooooooooooooooooooo
// long.
/* static */
void Foo::Bar() {
I prefer the return type on a separate line to the function name, but I can accept alternatives if that is not be a maintainable solution.
There are also other possibilities that have some benefits, but it may be debatable whether the benefits outweigh the disadvantages. e.g.
auto Foo::Bar(
RefPtr<mozilla::Baz> aTheVeryFirstParameterInThisFunction,
RefPtr<mozilla::Baz> aTheVeryLastParameterWithALongNameInThisFunction)
/* static */ -> alreadyAddRefed<mozilla::Baz> {
Comment 7•6 years ago
|
||
(In reply to Karl Tomlinson (:karlt) from comment #6)
(In reply to :Ehsan Akhgari from comment #5)
Well, to me it seems like the example in your comment shows that in this case we have to add a blank line.
The example was intended to demonstrate that clang-format folds
//
comments together into one comment, but does not do the same for/* */
comments.
Yes, I understand.
I can understand the benefit of a blank line when only
//
comments are used, but the blank line is not necessary with/* static */
.
Sure.
If I understand your objection, it's not that you're saying adding a blank line wouldn't solve the problem you're pointing out, it's that you're saying you don't like that solution, and you prefer instead to keep the current state of the world, which is keep the C-style /* static */ comments on the first line of function definitions. Do I understand your position correctly?
Partially.
You are correct in interpreting that I don't like having a blank line, thanks. i.e.// This comment is toooooooooooooooooooooooooooooooooooooooooooooooooooooooooo // long. // static void Foo::Bar() {
I also don't like the current state of the world with /* static */ on the same line as the function name. This is shifting the function name to the right, which can lead to truncation, but in some tools the opening comment means that some tools ignore the line altogether and show a previous function name. i.e.
// This comment is toooooooooooooooooooooooooooooooooooooooooooooooooooooooooo // long. /* static */ void Foo::Bar() {
Yes. Like hg/git diff apparently https://groups.google.com/d/msg/mozilla.dev.platform/6_reFXkmZIA/_ADlTOV8BgAJ. :-( That is arguably the biggest problem to solve here, beyond the stylistic issues IMO.
IMO this is a better solution to either of the above.
// This comment is toooooooooooooooooooooooooooooooooooooooooooooooooooooooooo // long. /* static */ void Foo::Bar() {
OK, thanks for explaining. I think this was the key part I was misunderstanding before. Ryan, what do you think? Can you live with this?
I prefer the return type on a separate line to the function name, but I can accept alternatives if that is not be a maintainable solution.
Yeah... About that (please feel free to open another bug if you feel like this is worth exploring in more detail):
On the thread you suggested using |//(clang-format line-break)|. I'm not sure if that was a serious suggestion or not, but to me that seems strictly worse than all options (I can't imagine having to teach people new to the code base that you have to relearn how to define a function just to be able to write code per our coding standard). And tbh so far I haven't received a ton of complaints about the return types being on the same line (could be because people just haven't bothered to complain, of course) so it's unclear how big of a problem this is for everyone. It certainly seems like something that other large projects have managed to live with.
There are also other possibilities that have some benefits, but it may be debatable whether the benefits outweigh the disadvantages. e.g.
auto Foo::Bar( RefPtr<mozilla::Baz> aTheVeryFirstParameterInThisFunction, RefPtr<mozilla::Baz> aTheVeryLastParameterWithALongNameInThisFunction) /* static */ -> alreadyAddRefed<mozilla::Baz> {
Yup, this was also suggested on the thread, as far as I understood in a joking way. I think this suggestion also suffers from the problem of having to teach people how to define a function. Furthermore, encouraging this syntax will create a situation where we'll have groups of people and parts of the code which will gravitate towards this way of defining functions and the majority of the code will still use the old-school syntax, and overall it will result in less consistency and more opportunities for people to wonder what's the right way to do things, argue over it, etc. It doesn't seem like a net positive change to me.
LMK if I'm missing something please! :-)
Assignee | ||
Comment 8•6 years ago
|
||
Sorry just catching up from PTO on this thread.
(In reply to :Ehsan Akhgari from comment #7)
(In reply to Karl Tomlinson (:karlt) from comment #6)
(In reply to :Ehsan Akhgari from comment #5)
IMO this is a better solution to either of the above.// This comment is toooooooooooooooooooooooooooooooooooooooooooooooooooooooooo // long. /* static */ void Foo::Bar() {
OK, thanks for explaining. I think this was the key part I was
misunderstanding before. Ryan, what do you think? Can you live with this?
So IIUC, this solution is finding a way to have c-style comments be on their own line before method definitions?
I'd be happy with that solution, but my (uninformed) impression was that this wasn't supported by clang-format.
I did a quick scan through the clang-format style options [1] and didn't see anything obvious, but I may have missed something.
[1] https://clang.llvm.org/docs/ClangFormatStyleOptions.html
Comment 9•6 years ago
|
||
Thanks, Ehsan. I think we understand each other about /* static */
now.
The return types on the same line are causing me pain due to the way tools truncate, but I agree we don't have a clear choice on how we could workaround that. The ideal solution would probably be to fix the tools, which is beyond the scope of this discussion.(In reply to Ryan Hunt [:rhunt] from comment #8)
(In reply to :Ehsan Akhgari from comment #7)
I'd be happy with that solution, but my (uninformed) impression was that this wasn't supported by clang-format.
clang-format is fine with leaving /* static */
on its own line. The confusion I think comes from the fact that clang-format tries to respect the original intent of /* static */ void
on the same line when it sees this in its input.
We avoid that if /* static */
is on its own line.
Comment 10•6 years ago
|
||
Yeah, exactly like Karl suggested. I think given the fact that the C++-style comment already has a newline at the end, all you need to do is to modify your regex syntax to create a C-style comment instead.
Assignee | ||
Comment 11•6 years ago
|
||
Okay, giving this a preliminary attempt.
Here's the regex used:
Find: (/*.**/) ([\w*\<>]+\s+\w+::)
Where: *.cpp
Replace: \1\n\2
The resulting commit [1].
If there's no bustage and no one has concerns with this, I'll look into splitting up patches for review.
[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=1480d886564d1dee293e8b0f2a4666099aef09b0
Assignee | ||
Comment 12•6 years ago
|
||
That patch had bustage from two macro definitions caught by the regex. Here's a green try with an extra patch to fix those changes. [1]
[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3b1455bd7b5952dadeb679dcde0968898a6b450
Assignee | ||
Comment 13•6 years ago
|
||
I've got patches split up for this that I'll put up for review [1]. They're all autogenerated using the following regex find and replace:
Find: (/*.**/) ([\w*\<>]+\s+\w+::)
Where: *.cpp
Replace: \1\n\2
For reference, as the desired formatting changed, this will convert from:
/* static */ void Foo::Bar() {
...
}
To:
/* static */
void Foo::Bar() {
...
}
There is one extra patch that fixes two cases where the regex interfered with some macro definitions. I'll squash that into the appropriate patch for landing.
I've done my best to find appropriate reviewers for each subdirectory, please redirect if someone is a better candidate. Ehsan, I've marked you as a reviewer for some changes I couldn't think of a better reviewer for, I hope that's okay.
[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=57c1ca780bec03a68586775929db0aa1ef1efe8a
Assignee | ||
Comment 14•6 years ago
|
||
Assignee | ||
Comment 15•6 years ago
|
||
Depends on D21101
Assignee | ||
Comment 16•6 years ago
|
||
Depends on D21102
Assignee | ||
Comment 17•6 years ago
|
||
Depends on D21103
Assignee | ||
Comment 18•6 years ago
|
||
Depends on D21104
Assignee | ||
Comment 19•6 years ago
|
||
Depends on D21105
Assignee | ||
Comment 20•6 years ago
|
||
Depends on D21106
Assignee | ||
Comment 21•6 years ago
|
||
Depends on D21107
Assignee | ||
Comment 22•6 years ago
|
||
Depends on D21108
Assignee | ||
Comment 23•6 years ago
|
||
Depends on D21109
Assignee | ||
Comment 24•6 years ago
|
||
Depends on D21110
Assignee | ||
Comment 25•6 years ago
|
||
Depends on D21111
Assignee | ||
Comment 26•6 years ago
|
||
Depends on D21112
Assignee | ||
Comment 27•6 years ago
|
||
Depends on D21114
Assignee | ||
Comment 28•6 years ago
|
||
Depends on D21115
Assignee | ||
Comment 29•6 years ago
|
||
Depends on D21116
Assignee | ||
Comment 30•6 years ago
|
||
Depends on D21117
Assignee | ||
Comment 31•6 years ago
|
||
Depends on D21118
Assignee | ||
Comment 32•6 years ago
|
||
Depends on D21119
Assignee | ||
Comment 33•6 years ago
|
||
Depends on D21120
Assignee | ||
Comment 34•6 years ago
|
||
Depends on D21121
Assignee | ||
Comment 35•6 years ago
|
||
Depends on D21122
Assignee | ||
Comment 36•6 years ago
|
||
Depends on D21123
Assignee | ||
Comment 37•6 years ago
|
||
Depends on D21124
Assignee | ||
Comment 38•6 years ago
|
||
Depends on D21125
Assignee | ||
Comment 39•6 years ago
|
||
Depends on D21128
Assignee | ||
Comment 40•6 years ago
|
||
Depends on D21130
Assignee | ||
Comment 41•6 years ago
|
||
Depends on D21131
Assignee | ||
Comment 42•6 years ago
|
||
Ugh, I forgot to do clang-format while building these changes, and then checked afterwards with ./mach clang-format -c baserev..toprev -s
, but it didn't work right with hg.
I'll update to fix the reviewbot warnings.
Assignee | ||
Comment 43•6 years ago
|
||
Here's a build-only try run with all the patches rebased on inbound [1].
[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6f96477232e7be6a0d81d12fbffd8a36d0c64d3
Comment 44•6 years ago
|
||
Comment 45•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2eff89bd9d57
https://hg.mozilla.org/mozilla-central/rev/cf1a37a3c469
https://hg.mozilla.org/mozilla-central/rev/02e1b59e669d
https://hg.mozilla.org/mozilla-central/rev/24f4ed60ba7f
https://hg.mozilla.org/mozilla-central/rev/4c547a6435fd
https://hg.mozilla.org/mozilla-central/rev/f41cee9bf149
https://hg.mozilla.org/mozilla-central/rev/2477669a9f35
https://hg.mozilla.org/mozilla-central/rev/5b5e6a994277
https://hg.mozilla.org/mozilla-central/rev/0a3f7bcc8145
https://hg.mozilla.org/mozilla-central/rev/240e874ed118
https://hg.mozilla.org/mozilla-central/rev/a683a7c54dc4
https://hg.mozilla.org/mozilla-central/rev/e16639cc628d
https://hg.mozilla.org/mozilla-central/rev/f99b937e9e7c
https://hg.mozilla.org/mozilla-central/rev/e0fb4657355d
https://hg.mozilla.org/mozilla-central/rev/e338646fe551
https://hg.mozilla.org/mozilla-central/rev/53d0d3dcfe6e
https://hg.mozilla.org/mozilla-central/rev/e850de3b6ac3
https://hg.mozilla.org/mozilla-central/rev/51a98c1a814f
https://hg.mozilla.org/mozilla-central/rev/76dd23f024d5
https://hg.mozilla.org/mozilla-central/rev/977a0ba7ec70
https://hg.mozilla.org/mozilla-central/rev/4ab919dd438b
https://hg.mozilla.org/mozilla-central/rev/4ec53f720e55
https://hg.mozilla.org/mozilla-central/rev/15132144bc25
https://hg.mozilla.org/mozilla-central/rev/9d07b7406ad2
https://hg.mozilla.org/mozilla-central/rev/a909cb872007
https://hg.mozilla.org/mozilla-central/rev/0707c5d27322
https://hg.mozilla.org/mozilla-central/rev/49477020b628
Updated•6 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Description
•