C++ method definition comments

[Ryan Hunt]

Hi all,

Quick C++ style question.

A common pattern in Gecko is for method definitions to have a comment with the
'static' or 'virtual' qualification.

Before the reformat, the comment would be on it's own separate line [1]. Now
it's on the main line of the definition [2].

For example:

/* static */ void
Foo::Bar() {
...
}

vs.

/* static */ void Foo::Bar() {
...
}

Personally I think this now takes too much horizontal space from the main
definition, and would prefer it to be either on its own line or just removed.

Does anyone have an opinion on whether we still want these comments? And if so
whether it makes sense to move them back to their own line.

(My ulterior motive is that sublime text's indexer started failing to find
these definitions after the reformat, but that should be fixed regardless)

If you're interested in what removing these would entail, I wrote a regex to
make the change [3].

Thanks,
Ryan

[1] https://hg.mozilla.org/mozilla-central/file/0348d472115d/layout/generic/nsFrame.cpp#l1759
[2] https://hg.mozilla.org/mozilla-central/file/e4b9b1084292/layout/generic/nsFrame.cpp#l1756
[3] https://hg.mozilla.org/try/rev/31ab3e466b6f15dcdbb1aee47edabc7c358b86f2

[Jonathan Watt]

Personally I find them useful. Putting them on a separate line seems reasonable
to me.

Jonathan

[David Teller]

I find them extremely useful, too (as in "removing them would make my
life miserable in quite a few bugs"). I have no problem with putting
them on a separate line.

Cheers,
David

[Ehsan Akhgari]

This is indeed one of the cases where the reformat has made things worse.
I think as a couple of people have already said, we'll find that some
people do find these annotations useful, even if they're not always
consistently present.

The path to least resistance for addressing this problem may be to convert
these into C++-style comments and therefore moving them into their own
lines. Would you be OK with that?

Thanks,
Ehsan

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>


--
Ehsan

[Jeff Gilbert]

I would much rather revert to:
/*static*/ void
Foo::Bar()

The Foo::Bar is the most relevant part of that whole expression, which
makes it nice to keep up against the start of the line.

In a clang-format world, we should feel more free to make such
deviations from Google Style, since it's all handled for us.

[Ryan Hunt]

Yeah, personally I have found them be useful and don't have an issue with keeping
them. I just wasn't sure if that was a common experience.

So for converting from C-style to C++-style, that would be:

/* static */ void Foo::Bar() {
...
}

// static
void Foo::Bar() {
...
}

I think that would be good. My one concern would be the presence of other C++-style
comments before the method definition. For example [1].

Ideally documentation like that should go in the header by the method declaration, but I
have no idea if we consistently do that.

[1] https://hg.mozilla.org/mozilla-central/file/e4b9b1084292/layout/generic/nsFrame.cpp#l1023

Thanks,
Ryan

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐

[gsqu...@mozilla.com]

Just a thought: Would it be worth considering a blank macro, e.g.:
static void foo();
DECLARED_STATIC void foo() {...}

On top of not being confused with other comments around, it could be clang-checked so it's never wrong. (And maybe eventually enforced, like MOZ_IMPLICIT is.)

Cheers,
Gerald

On Tuesday, January 29, 2019 at 10:27:17 AM UTC+11, Ryan Hunt wrote:
> Yeah, personally I have found them be useful and don't have an issue with keeping
> them. I just wasn't sure if that was a common experience.
>
> So for converting from C-style to C++-style, that would be:
>
> /* static */ void Foo::Bar() {
> ...
> }
>
> // static
> void Foo::Bar() {
> ...
> }
>
> I think that would be good. My one concern would be the presence of other C++-style
> comments before the method definition. For example [1].
>
> Ideally documentation like that should go in the header by the method declaration, but I
> have no idea if we consistently do that.
>
> [1] https://hg.mozilla.org/mozilla-central/file/e4b9b1084292/layout/generic/nsFrame.cpp#l1023
>
> Thanks,
> Ryan
>
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐

> > --
> > Ehsan

[Ehsan Akhgari]

On Mon, Jan 28, 2019 at 6:27 PM Ryan Hunt <rh...@eqrion.net> wrote:

> Yeah, personally I have found them be useful and don't have an issue with
> keeping
> them. I just wasn't sure if that was a common experience.
>
> So for converting from C-style to C++-style, that would be:
>
> /* static */ void Foo::Bar() {
> ...
> }
>
> // static
> void Foo::Bar() {
> ...
> }
>
>
>
> I think that would be good.
>

Great!

> My one concern would be the presence of other C++-style
> comments before the method definition. For example [1].
>

That's nothing that a bit of regex wizardry can't take care of. :-) How
about detecting those cases and inserting a newline between the comments on
the line before, for extra clarity?

> Ideally documentation like that should go in the header by the method
> declaration, but I
> have no idea if we consistently do that.
>
> [1]
> https://hg.mozilla.org/mozilla-central/file/e4b9b1084292/layout/generic/nsFrame.cpp#l1023
>
> Thanks,
> Ryan
>
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On Monday, January 28, 2019 12:51 PM, Ehsan Akhgari <

> ehsan....@gmail.com> wrote:
>
> This is indeed one of the cases where the reformat has made things worse.
> I think as a couple of people have already said, we'll find that some
> people do find these annotations useful, even if they're not always
> consistently present.
>
> The path to least resistance for addressing this problem may be to convert
> these into C++-style comments and therefore moving them into their own
> lines. Would you be OK with that?
>
> Thanks,
> Ehsan
>

>> _______________________________________________
>> dev-platform mailing list
>> dev-pl...@lists.mozilla.org
>> https://lists.mozilla.org/listinfo/dev-platform
>>
>
>
> --
> Ehsan
>
>
>

--
Ehsan

[Ehsan Akhgari]

On Mon, Jan 28, 2019 at 7:10 PM <gsqu...@mozilla.com> wrote:

> Just a thought: Would it be worth considering a blank macro, e.g.:
> static void foo();
> DECLARED_STATIC void foo() {...}
>
> On top of not being confused with other comments around, it could be
> clang-checked so it's never wrong. (And maybe eventually enforced, like
> MOZ_IMPLICIT is.)
>

Yeah, that could be a future alternative, but it would require someone to
do the hard work of implementing the required static analysis and landing
it. I think Ryan's proposal will probably simplify that process somewhat
by making it possible to mass-replace a bunch of "// static" comments with
"DECLARED_STATIC" or some such, but I don't want to hold the good solution
here for a perfect solution in the future. I would personally be OK for
these two to happen incrementally.

Would you mind filing a bug for this please?

Thanks,
Ehsan

[Ehsan Akhgari]

On Mon, Jan 28, 2019 at 2:58 PM Jeff Gilbert <jgil...@mozilla.com> wrote:

> I would much rather revert to:
> /*static*/ void
> Foo::Bar()
>
> The Foo::Bar is the most relevant part of that whole expression, which
> makes it nice to keep up against the start of the line.
>

The clang-format option which allows formatting the way you are suggesting,
AlwaysBreakAfterDefinitionReturnType, is deprecated, and is likely to be
removed from a future version of clang-format, so there is no sustainable
way for us to adopt this suggestion.

> In a clang-format world, we should feel more free to make such
> deviations from Google Style, since it's all handled for us.
>
> On Mon, Jan 28, 2019 at 10:52 AM Ehsan Akhgari <ehsan....@gmail.com>

[gsqu...@mozilla.com]

On Wednesday, January 30, 2019 at 9:57:02 AM UTC+11, Ehsan Akhgari wrote:
> On Mon, Jan 28, 2019 at 7:10 PM <gsqu...@mozilla.com> wrote:
>
> > Just a thought: Would it be worth considering a blank macro, e.g.:
> > static void foo();
> > DECLARED_STATIC void foo() {...}
> >
> > On top of not being confused with other comments around, it could be
> > clang-checked so it's never wrong. (And maybe eventually enforced, like
> > MOZ_IMPLICIT is.)
> >
>
> Yeah, that could be a future alternative, but it would require someone to
> do the hard work of implementing the required static analysis and landing
> it. I think Ryan's proposal will probably simplify that process somewhat
> by making it possible to mass-replace a bunch of "// static" comments with
> "DECLARED_STATIC" or some such, but I don't want to hold the good solution
> here for a perfect solution in the future. I would personally be OK for
> these two to happen incrementally.
>
> Would you mind filing a bug for this please?
>
> Thanks,
> Ehsan

Thank you Ehsan. Yes, a phased approach would definitely be the way to go.
https://bugzilla.mozilla.org/show_bug.cgi?id=1523793

I realize now that this doesn't help at all with the issue of valuable horizontal real-estate! Sorry for the tangent.

One (partly tongue-in-cheek) solution to make the important function name more prominent is to use trailing return types:
`auto Foo::Bar() -> void {`
Note that this is more easily greppable when searching for function names.
And it looks more like Rust.

To help with spacing, the `DECLARED_...` macros could be placed at the end (if possible?) :
`void Foo::Bar() DECLARED_STATIC {`
`auto Foo::Bar() -> void DECLARED_STATIC {`
But this feels uglier to me.

> --
> Ehsan

[Karl Tomlinson]

Ehsan Akhgari writes:

> On Mon, Jan 28, 2019 at 6:27 PM Ryan Hunt <rh...@eqrion.net> wrote:
>

>> [...]

>>
>> So for converting from C-style to C++-style, that would be:
>>
>> /* static */ void Foo::Bar() {
>> ...
>> }
>>
>> // static
>> void Foo::Bar() {
>> ...
>> }
>>

>> [...]

>>
>> My one concern would be the presence of other C++-style
>> comments before the method definition. For example [1].
>

> [...] How about detecting those cases and inserting a newline

> between the comments on the line before, for extra clarity?
>

>> [...]
>>
>> [1]
>> https://hg.mozilla.org/mozilla-central/file/e4b9b1084292/layout/generic/nsFrame.cpp#l1023

>>
>> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
>> On Monday, January 28, 2019 12:51 PM, Ehsan Akhgari <
>> ehsan....@gmail.com> wrote:
>>

>> [...]

>>
>> The path to least resistance for addressing this problem may be to convert
>> these into C++-style comments and therefore moving them into their own
>> lines. Would you be OK with that?

I haven't noticed clang-format enforcing its own opinions on
comments when they already follow Google style.

In my experiments clang-format is accepting this:

// Make this Foo Bar.

/* static */
void Foo::Bar() {
...
}

The /* */ style comment provides a clear separation from any other
comment on the previous line, without the need for an extra
blank-line. "don't use blank lines when you don't have to."

[Karl Tomlinson]

Ehsan Akhgari writes:

> On Mon, Jan 28, 2019 at 2:58 PM Jeff Gilbert <jgil...@mozilla.com> wrote:
>
>> I would much rather revert to:
>> /*static*/ void
>> Foo::Bar()
>>
>> The Foo::Bar is the most relevant part of that whole expression, which
>> makes it nice to keep up against the start of the line.
>>
>
> The clang-format option which allows formatting the way you are suggesting,
> AlwaysBreakAfterDefinitionReturnType, is deprecated, and is likely to be
> removed from a future version of clang-format, so there is no sustainable
> way for us to adopt this suggestion.

Where there's a will there's often a way. e.g.

/*static*/ void //(clang-format line-break)
Foo::Bar() {

I do like being able to see function names in diff output, but I'm
not so keen on having to put //cflb at the beginning of every
function. This feels too much like working against the decision
to follow Google style.

With so much code using Google style, I guess there must be tools
to show useful information in diff output, or at least there will
be soon...

[Ehsan Akhgari]

Oh but I think it will actually. :-) clang-format has a heuristic that
makes it put upper-case macros before function definition return types on
their own lines (e.g. see how our NS_IMETHODIMP macro is treated.)

> One (partly tongue-in-cheek) solution to make the important function name
> more prominent is to use trailing return types:
> `auto Foo::Bar() -> void {`
> Note that this is more easily greppable when searching for function names.
> And it looks more like Rust.
>
> To help with spacing, the `DECLARED_...` macros could be placed at the end
> (if possible?) :
> `void Foo::Bar() DECLARED_STATIC {`
> `auto Foo::Bar() -> void DECLARED_STATIC {`
> But this feels uglier to me.
>

Interesting idea! I wonder if we are at a point in our adoption curve of
Rust that we should worry about how weird our C++ code looks for the
majority of developers yet... I suppose doing this would mean reserving
only five characters at the beginning of function names for the "return
type".

[Ehsan Akhgari]

It depends on where you start from. If you start from the code sample
above, clang-format won't touch the lines with comments. However if you
start from the code sample below, it will:

// Make this Foo Bar.
/* static */ void
Foo::Bar()
{

// ...
}

It will get reformatted to:

// Make this Foo Bar.
/* static */ void Foo::Bar() {

// ...
}

Cheers,
--
Ehsan

[Ehsan Akhgari]

What tool do you use which has difficulty showing function names in diffs
right now? It seems to work fine for me both in git and hgweb...

--
Ehsan

[Ryan Hunt]

I've filed bug 1523969 to consider making this change.

(https://bugzilla.mozilla.org/show_bug.cgi?id=1523969)

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐

On Monday, January 28, 2019 6:27 PM, Ryan Hunt <rh...@eqrion.net> wrote:

> Yeah, personally I have found them be useful and don't have an issue with keeping
> them. I just wasn't sure if that was a common experience.
>

> So for converting from C-style to C++-style, that would be:
>
> /* static */ void Foo::Bar() {
> ...
> }
>
> // static
> void Foo::Bar() {
> ...
> }
>

> I think that would be good. My one concern would be the presence of other C++-style

> comments before the method definition. For example [1].
>

> Ideally documentation like that should go in the header by the method declaration, but I
> have no idea if we consistently do that.
>
> [1] https://hg.mozilla.org/mozilla-central/file/e4b9b1084292/layout/generic/nsFrame.cpp#l1023
>
> Thanks,
> Ryan
>

> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On Monday, January 28, 2019 12:51 PM, Ehsan Akhgari <ehsan....@gmail.com> wrote:
>

>> This is indeed one of the cases where the reformat has made things worse. I think as a couple of people have already said, we'll find that some people do find these annotations useful, even if they're not always consistently present.
>>

>> The path to least resistance for addressing this problem may be to convert these into C++-style comments and therefore moving them into their own lines. Would you be OK with that?
>>

>> Thanks,
>> Ehsan

>>
>> On Fri, Jan 25, 2019 at 11:49 PM Ryan Hunt <rh...@eqrion.net> wrote:
>>
>>> Hi all,
>>>
>>> Quick C++ style question.
>>>
>>> A common pattern in Gecko is for method definitions to have a comment with the
>>> 'static' or 'virtual' qualification.
>>>
>>> Before the reformat, the comment would be on it's own separate line [1]. Now
>>> it's on the main line of the definition [2].
>>>
>>> For example:
>>>

>>> /* static */ void
>>> Foo::Bar() {
>>> ...
>>> }
>>>

>>> vs.

>>>
>>> /* static */ void Foo::Bar() {
>>> ...
>>> }
>>>

>>> Personally I think this now takes too much horizontal space from the main
>>> definition, and would prefer it to be either on its own line or just removed.
>>>
>>> Does anyone have an opinion on whether we still want these comments? And if so
>>> whether it makes sense to move them back to their own line.
>>>
>>> (My ulterior motive is that sublime text's indexer started failing to find
>>> these definitions after the reformat, but that should be fixed regardless)
>>>
>>> If you're interested in what removing these would entail, I wrote a regex to
>>> make the change [3].
>>>
>>> Thanks,
>>> Ryan
>>>
>>> [1] https://hg.mozilla.org/mozilla-central/file/0348d472115d/layout/generic/nsFrame.cpp#l1759
>>> [2] https://hg.mozilla.org/mozilla-central/file/e4b9b1084292/layout/generic/nsFrame.cpp#l1756
>>> [3] https://hg.mozilla.org/try/rev/31ab3e466b6f15dcdbb1aee47edabc7c358b86f2
>>>

[Karl Tomlinson]

Ehsan Akhgari writes:

> What tool do you use which has difficulty showing function names in diffs
> right now? It seems to work fine for me both in git and hgweb...

It's cases like these that are truncated earlier due to putting
the return type before the function name:

% hg export 9f7b93f5c4f8 | sed -n 275p
@@ -891,19 +886,17 @@ nsresult nsMixedContentBlocker::ShouldLo
% hg export b8d2dfdfc324 | sed -n 2094p
@@ -1717,16 +1860,23 @@ already_AddRefed<nsIFactory> nsFactoryEn

https://hg.mozilla.org/mozilla-central/rev/9f7b93f5c4f8#l5.119
https://hg.mozilla.org/mozilla-central/rev/b8d2dfdfc324#l7.838

[Boris Zbarsky]

On 1/30/19 10:19 AM, Ehsan Akhgari wrote:
> What tool do you use which has difficulty showing function names in diffs
> right now? It seems to work fine for me both in git and hgweb...

"hg diff" and "git diff" both fail at this over here when there is a /*
comment */ before the function name. hg version 4.4.2 and git version
2.14.5.

For example, if you make a change in the middle of
nsContentUtils::AttemptLargeAllocationLoad and then hg diff or git diff,
it will claim your change is in
nsContentUtils::LookupCustomElementDefinition. I just ran into this
while trying to read a patch someone asked for review on.

If there is no comment before the function name, it works.

-Boris

[Ryan Hunt]

Quick update.

It looks like clang-format is actually okay with C-style comments being on
their own line before a method definition. So last night patches landed to move
all these comments to their own line.

From:
/* static */ void Foo::Bar() { ... }

To:

/* static */
void Foo::Bar() { ... }

So why did the initial reformatting move all these comments around?

I believe the issue is that before the reformat, the C-style comments were on
the same line as the return types of the methods [1]. I think this lead
clang-format to view the comments as being part of the method definitions, and
so they were all formatted to be on the same line.

[1] https://searchfox.org/mozilla-central/diff/265e6721798a455604328ed5262f430cfcc37c2f/layout/base/nsLayoutUtils.cpp#1098

Thanks,
Ryan

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐