Re: C++ method definition comments
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 ‐‐‐ On Wednesday, January 30, 2019 10:17 AM, Ryan Hunt wrote: > 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 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 >> 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 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 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform >>> >>> -- >>> Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: C++ method definition comments
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 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: C++ method definition comments
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 nsFactoryEn https://hg.mozilla.org/mozilla-central/rev/9f7b93f5c4f8#l5.119 https://hg.mozilla.org/mozilla-central/rev/b8d2dfdfc324#l7.838 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: C++ method definition comments
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 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 > 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 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 >>> >>> ___ >>> dev-platform mailing list >>> dev-platform@lists.mozilla.org >>> https://lists.mozilla.org/listinfo/dev-platform >> >> -- >> Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: C++ method definition comments
On Wed, Jan 30, 2019 at 1:35 AM Karl Tomlinson wrote: > Ehsan Akhgari writes: > > > On Mon, Jan 28, 2019 at 2:58 PM Jeff Gilbert > 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... > 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 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: C++ method definition comments
On Wed, Jan 30, 2019 at 1:30 AM Karl Tomlinson wrote: > Ehsan Akhgari writes: > > > On Mon, Jan 28, 2019 at 6:27 PM Ryan Hunt 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.akhg...@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." > 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 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: C++ method definition comments
On Tue, Jan 29, 2019 at 6:40 PM wrote: > On Wednesday, January 30, 2019 at 9:57:02 AM UTC+11, Ehsan Akhgari wrote: > > On Mon, Jan 28, 2019 at 7:10 PM 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. > 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". > 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 ‐‐‐ > > > > 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 > 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
Re: C++ method definition comments
Ehsan Akhgari writes: > On Mon, Jan 28, 2019 at 2:58 PM Jeff Gilbert 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... ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: C++ method definition comments
Ehsan Akhgari writes: > On Mon, Jan 28, 2019 at 6:27 PM Ryan Hunt 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.akhg...@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." ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: C++ method definition comments
On Wednesday, January 30, 2019 at 9:57:02 AM UTC+11, Ehsan Akhgari wrote: > On Mon, Jan 28, 2019 at 7:10 PM 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. 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 ‐‐‐ > > > On Monday, January 28, 2019 12:51 PM, Ehsan Akhgari > > 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 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 > > > >> > > > > > > > > -- > > > > Ehsan > > > > > -- > Ehsan
Re: C++ method definition comments
On Mon, Jan 28, 2019 at 7:10 PM 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 > 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 ‐‐‐ > > On Monday, January 28, 2019 12:51 PM, Ehsan Akhgari > 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 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 > > >> > > > > > > -- > > > Ehsan > ___ > dev-platform mailing list > dev-platform@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-platform > -- Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: C++ method definition comments
On Mon, Jan 28, 2019 at 2:58 PM Jeff Gilbert 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 > 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 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 > > > > > > ___ > > > dev-platform mailing list > > > dev-platform@lists.mozilla.org > > > https://lists.mozilla.org/listinfo/dev-platform > > > > > > > > > -- > > Ehsan > > ___ > > dev-platform mailing list > > dev-platform@lists.mozilla.org > > https://lists.mozilla.org/listinfo/dev-platform > -- Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: C++ method definition comments
On Mon, Jan 28, 2019 at 6:27 PM 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. > 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.akhg...@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 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 >> >> ___ >> dev-platform mailing list >> dev-platform@lists.mozilla.org >> https://lists.mozilla.org/listinfo/dev-platform >> > > > -- > Ehsan > > > -- Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: C++ method definition comments
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 ‐‐‐ > On Monday, January 28, 2019 12:51 PM, Ehsan Akhgari > 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 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 > >> > > > > -- > > Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: C++ method definition comments
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 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 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 >> >> ___ >> dev-platform mailing list >> dev-platform@lists.mozilla.org >> https://lists.mozilla.org/listinfo/dev-platform > > -- > Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: C++ method definition comments
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. On Mon, Jan 28, 2019 at 10:52 AM Ehsan Akhgari 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 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 > > > > ___ > > dev-platform mailing list > > dev-platform@lists.mozilla.org > > https://lists.mozilla.org/listinfo/dev-platform > > > > > -- > Ehsan > ___ > dev-platform mailing list > dev-platform@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: C++ method definition comments
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 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 > > ___ > dev-platform mailing list > dev-platform@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-platform > -- Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: C++ method definition comments
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 On 26/01/2019 15:19, Jonathan Watt wrote: > Personally I find them useful. Putting them on a separate line seems > reasonable > to me. > > Jonathan > ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: C++ method definition comments
Personally I find them useful. Putting them on a separate line seems reasonable to me. Jonathan On 26/01/2019 04:49, Ryan Hunt 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 > ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
C++ method definition comments
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 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform