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

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."

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.

(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?

(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> {

(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! :-)

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

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.

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.

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

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

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

Depends on D21101

Depends on D21102

Depends on D21103

Depends on D21104

Depends on D21105

Depends on D21106

Depends on D21107

Depends on D21108

Depends on D21109

Depends on D21110

Depends on D21111

Depends on D21112

Depends on D21114

Depends on D21115

Depends on D21116

Depends on D21117

Depends on D21118

Depends on D21119

Depends on D21120

Depends on D21121

Depends on D21122

Depends on D21123

Depends on D21124

Depends on D21125

Depends on D21128

Depends on D21130

Depends on D21131

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.

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

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