Re: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
Will there be a blanket change sometime soon? If not, are we supposed to start using the new style in patches now, even when it would clash with nearby old-style overrides/finals in the same file or in the same directory? ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
On 2015-05-13 10:03 AM, Benjamin Smedberg wrote: I think it's time I made an official decision here. I think Ehsan's proposal makes a lot of sense both as good engineering and because we know Google has already proved it for us. I approve. Ehsan, will you please make the necessary changes to our style guide? Done: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style$compare?locale=en-USto=802537from=802167 Thanks, Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
I think it's time I made an official decision here. I think Ehsan's proposal makes a lot of sense both as good engineering and because we know Google has already proved it for us. I approve. Ehsan, will you please make the necessary changes to our style guide? --BDS On Mon, Apr 27, 2015 at 3:48 PM, Ehsan Akhgari ehsan.akhg...@gmail.com wrote: Right now, our coding style requires that both the virtual and override keywords to be specified for overridden virtual functions. A few things have changed since we decided that a number of years ago: 1. The override and final keywords are now available on all of the compilers that we build with, and we have stopped supporting compilers that do not support these features. 2. We have very recently gained the ability to run clang-based mass source code transformations over our code that would let us enforce the coding style [1]. I would like to propose a change to our coding style, and run a mass transformation over our code in order to enforce it. I think we should change it to require the usage of exactly one of these keywords per *overridden* function: virtual, override, and final. Here are the advantages: * It is a more succinct, as |virtual void foo() override;| doesn't convey more information than |void foo() override;|. * It can be easily enforced using clang-tidy across the entire code base. * It makes it easier to determine what kind of function you are looking at by just looking at its declaration. |virtual void foo();| means a virtual function that is not overridden, |void foo() override;| means an overridden virtual function, and |void foo() final;| means an overridden virtual function that cannot be further overridden. * It allows us to be in sync with the Google C++ Style on this issue [2]. * It will allow us to remove NS_IMETHODIMP, and use NS_IMETHOD instead. Please let me know what you think! [1] Facilitated by bug 904572. Also see bug 1158776 for my first attempt at this. [2] http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance Cheers, -- Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform -- Benjamin Smedberg Engineering Manager, Firefox ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
On 05/07/2015 02:53 PM, Karl Tomlinson wrote: The warning that you are proposing to fix here is -Woverloaded-virtual. [EDIT: karl meant to say -Winconsistent-missing-override] At least once we can build with this warning enabled, I recommend making this warning fatal instead of covering over it by adding an override annotation that the author may have never intended. Semi-tangent, to correct one premise here: Good news -- we already *can do* build with this warning (-Winconsistent-missing-override) enabled, and it's fatal in FAIL_ON_WARNINGS directories which is most of the tree. bug 1117034 tracks a lot of the fixes for that. You just need clang 3.6 or newer to get this warning, and our official TreeHerder clang builders have an older version, so they don't report this warning. As a result, we get a few new instances checked in per week -- but I've been catching those locally (since they bust my build) and I've been fixing them as they crop up. (But anyway, as ehsan replied separately, his proposed coding style change isn't about fixing instances of this build warning.) /semi-tangent ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
On 2015-05-07 5:53 PM, Karl Tomlinson wrote: Ehsan Akhgari writes: This seems similar to the compiler warning situation. Usually at least, I don't think we should automatically modify the code in line with how the compiler reads the code just to silence the warning. Instead the warning is there to indicate that a programmer needs to check the code. These cases bear no similarity whatsoever. I can't think of any compiler warnings that can be automatically fixed without changing the meaning of the program. The warning that you are proposing to fix here is -Woverloaded-virtual. No. My proposal is completely unrelated to function overloading. Perhaps you're thinking of clang's -Winconsistent-missing-override? This proposal obviously fixes that warning (as a mere side effect), but that warning wouldn't catch everything that this proposal covers. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
Ehsan Akhgari writes: On 2015-05-07 5:53 PM, Karl Tomlinson wrote: Ehsan Akhgari writes: This seems similar to the compiler warning situation. Usually at least, I don't think we should automatically modify the code in line with how the compiler reads the code just to silence the warning. Instead the warning is there to indicate that a programmer needs to check the code. These cases bear no similarity whatsoever. I can't think of any compiler warnings that can be automatically fixed without changing the meaning of the program. The warning that you are proposing to fix here is -Woverloaded-virtual. No. My proposal is completely unrelated to function overloading. Perhaps you're thinking of clang's -Winconsistent-missing-override? This proposal obviously fixes that warning (as a mere side effect), but that warning wouldn't catch everything that this proposal covers. Ah, yes. Sorry to confusing things. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
On 2015-04-29 9:16 PM, Karl Tomlinson wrote: and this one isn't final, but we could make it final if the tu will be built into libxul (because then devirtualization is fine) http://mxr.mozilla.org/mozilla-central/source/dom/base/nsIDocument.h#1287 I'm not sure why that function is virtual. If it's just in order to make it possible to call it through a vtable from outside of libxul, I'm not sure why we need to treat it differently than other XPCOM APIs for example. If this is not used outside of libxul, we can probably make it non-virtual. XPCOM APIs are usually abstract interfaces and so not final. final could be removed and have valid code, but may be a useful optimization or enforcement of intent. virtual final seems necessary for some use cases, and there is no redundancy. Is there a need to ban this usage? The point is that there are *very few* use cases. Right now it seems that there is only one place in mozilla-central where we use a non-overriding virtual function that is also final, and that is a function that is never called by anybody. In almost every other case, such a method would just be devirtualized. The case ththat Trevor mentioned above (using a non-virtual function for internal code but allowing external code to access it too) is easily served by a virtual non-final function that wraps a non-virtual one that the internal code uses. The point of having a coding style is to have it prescribe what is desirable in 99% of the cases, not prescribe something that works for every single edge case that most people should never have to use. :-) ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
On 2015-05-05 2:51 PM, Jeff Walden wrote: Seeing this a touch late, commenting on things not noted yet. On 04/27/2015 12:48 PM, Ehsan Akhgari wrote: I think we should change it to require the usage of exactly one of these keywords per *overridden* function: virtual, override, and final. Here are the advantages: * It is a more succinct, as |virtual void foo() override;| doesn't convey more information than |void foo() override;|. * It makes it easier to determine what kind of function you are looking at by just looking at its declaration. |virtual void foo();| means a virtual function that is not overridden, |void foo() override;| means an overridden virtual function, and |void foo() final;| means an overridden virtual function that cannot be further overridden. All else equal, shorter is better. But this concision hurts readability, even past the non-obvious final/override = virtual implication others have noted. (And yes, C++ can/should permit final/override on non-virtuals. JSString and subclasses would be immediate users.) At the risk of repeating myself and others here, let's not worry about what C++ should do if it were being redesigned today, and let's focus on what it actually does. Requiring removal of virtual from the signature for final/override prevents simply examining a declaration's start to determine whether the function is virtual. You must read the entire declaration to know: a problem because final/override can blend in. For longer (especially multiline) declarations this matters. Consider these SpiderMonkey declarations: /* Standard internal methods. */ virtual bool getOwnPropertyDescriptor(JSContext* cx, HandleObject proxy, HandleId id, MutableHandleJSPropertyDescriptor desc) const override; virtual bool defineProperty(JSContext* cx, HandleObject proxy, HandleId id, HandleJSPropertyDescriptor desc, ObjectOpResult result) const override; virtual bool ownPropertyKeys(JSContext* cx, HandleObject proxy, AutoIdVector props) const override; virtual bool delete_(JSContext* cx, HandleObject proxy, HandleId id, ObjectOpResult result) const override; virtual is extraordinarily clear in starting each declaration. override and final alone would be obscured at the end of a long string of text, especially when skimming. (I disagree with assuming syntax coloring penalizing non-IDE users.) This is basically what Trevor was arguing for. I don't think there is much more to say on this as we're basically comparing what you and I are used to read. * It will allow us to remove NS_IMETHODIMP, and use NS_IMETHOD instead. A better alternative, that exposes standard C++ and macro-hides non-standard gunk, would be to use NS_IMETHOD for everything and directly use |virtual| in declarations. This point is no justification for changing style rules. That has similar verbosity issues. But yeah I agree on the broader point that this macro situation can be solved in other ways, we just disagree what those ways should be. :-) ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
On 2015-04-29 9:17 PM, Karl Tomlinson wrote: Ehsan Akhgari writes: I think there's a typo of some sort in the question, but if you meant every overriding function must be marked with override, then yes, that is the change I'm proposing, but the good news is that you can now run clang-tidy on the entire tree and get it to rewrite the source code to make sure that the override keywords are present where they are needed, and we can do that as often as we would like. IOW, this can be done without requiring every C++ programmer to remember to do it always. I fear that an automatic update would be more than just enforcing a style because override keywords imply programmer intent. If the proposal is to periodically automatically add override keywords where methods override but are currently not annotated as such Yes, I would like us to get to that point (but running the tool needs to be done manually for now.) then it seems we should we have an annotation to indicate that a method is not intended to override. However, that would require annotating all methods. I don't understand what you're suggesting. Adding override to an overriding virtual function doesn't change what the program means. There is no need to annotate all methods. (Please note that my proposal works very well in other large well established code bases. I'm not exactly making up a new rule!) This seems similar to the compiler warning situation. Usually at least, I don't think we should automatically modify the code in line with how the compiler reads the code just to silence the warning. Instead the warning is there to indicate that a programmer needs to check the code. These cases bear no similarity whatsoever. I can't think of any compiler warnings that can be automatically fixed without changing the meaning of the program. Perhaps though there is a case for a one-off change to add override automatically so that warnings can be enabled on new code. Again, I'm not sure what you're mentioning exactly, but my proposal entails periodic automatic rewrites of the code without changing what the code means. And hopefully people will start to gradually obey the new coding style. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
On Fri, May 08, 2015 at 09:53:54AM +1200, Karl Tomlinson wrote: Ehsan Akhgari writes: This seems similar to the compiler warning situation. Usually at least, I don't think we should automatically modify the code in line with how the compiler reads the code just to silence the warning. Instead the warning is there to indicate that a programmer needs to check the code. These cases bear no similarity whatsoever. I can't think of any compiler warnings that can be automatically fixed without changing the meaning of the program. The warning that you are proposing to fix here is -Woverloaded-virtual. At least once we can build with this warning enabled, I recommend making this warning fatal instead of covering over it by adding an override annotation that the author may have never intended. It *is* enabled and fatal, but there are a few places that disable it. Mike ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
Ehsan Akhgari writes: This seems similar to the compiler warning situation. Usually at least, I don't think we should automatically modify the code in line with how the compiler reads the code just to silence the warning. Instead the warning is there to indicate that a programmer needs to check the code. These cases bear no similarity whatsoever. I can't think of any compiler warnings that can be automatically fixed without changing the meaning of the program. The warning that you are proposing to fix here is -Woverloaded-virtual. At least once we can build with this warning enabled, I recommend making this warning fatal instead of covering over it by adding an override annotation that the author may have never intended. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
A request from the docs team: once the final decisions are made, please either let us know what those decisions are (use our doc request form: https://bugzilla.mozilla.org/form.doc) or update the coding style guide yourselves ( https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style). Either one wins you brownie points, but updating the doc yourselves will result in super-tasty brownie points. On Thu, May 7, 2015 at 9:52 AM, Ehsan Akhgari ehsan.akhg...@gmail.com wrote: On 2015-05-05 2:51 PM, Jeff Walden wrote: Seeing this a touch late, commenting on things not noted yet. On 04/27/2015 12:48 PM, Ehsan Akhgari wrote: I think we should change it to require the usage of exactly one of these keywords per *overridden* function: virtual, override, and final. Here are the advantages: * It is a more succinct, as |virtual void foo() override;| doesn't convey more information than |void foo() override;|. * It makes it easier to determine what kind of function you are looking at by just looking at its declaration. |virtual void foo();| means a virtual function that is not overridden, |void foo() override;| means an overridden virtual function, and |void foo() final;| means an overridden virtual function that cannot be further overridden. All else equal, shorter is better. But this concision hurts readability, even past the non-obvious final/override = virtual implication others have noted. (And yes, C++ can/should permit final/override on non-virtuals. JSString and subclasses would be immediate users.) At the risk of repeating myself and others here, let's not worry about what C++ should do if it were being redesigned today, and let's focus on what it actually does. Requiring removal of virtual from the signature for final/override prevents simply examining a declaration's start to determine whether the function is virtual. You must read the entire declaration to know: a problem because final/override can blend in. For longer (especially multiline) declarations this matters. Consider these SpiderMonkey declarations: /* Standard internal methods. */ virtual bool getOwnPropertyDescriptor(JSContext* cx, HandleObject proxy, HandleId id, MutableHandleJSPropertyDescriptor desc) const override; virtual bool defineProperty(JSContext* cx, HandleObject proxy, HandleId id, HandleJSPropertyDescriptor desc, ObjectOpResult result) const override; virtual bool ownPropertyKeys(JSContext* cx, HandleObject proxy, AutoIdVector props) const override; virtual bool delete_(JSContext* cx, HandleObject proxy, HandleId id, ObjectOpResult result) const override; virtual is extraordinarily clear in starting each declaration. override and final alone would be obscured at the end of a long string of text, especially when skimming. (I disagree with assuming syntax coloring penalizing non-IDE users.) This is basically what Trevor was arguing for. I don't think there is much more to say on this as we're basically comparing what you and I are used to read. * It will allow us to remove NS_IMETHODIMP, and use NS_IMETHOD instead. A better alternative, that exposes standard C++ and macro-hides non-standard gunk, would be to use NS_IMETHOD for everything and directly use |virtual| in declarations. This point is no justification for changing style rules. That has similar verbosity issues. But yeah I agree on the broader point that this macro situation can be solved in other ways, we just disagree what those ways should be. :-) ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform -- Eric Shepherd Senior Technical Writer Mozilla Blog: http://www.bitstampede.com/ Twitter: http://twitter.com/sheppy ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
Seeing this a touch late, commenting on things not noted yet. On 04/27/2015 12:48 PM, Ehsan Akhgari wrote: I think we should change it to require the usage of exactly one of these keywords per *overridden* function: virtual, override, and final. Here are the advantages: * It is a more succinct, as |virtual void foo() override;| doesn't convey more information than |void foo() override;|. * It makes it easier to determine what kind of function you are looking at by just looking at its declaration. |virtual void foo();| means a virtual function that is not overridden, |void foo() override;| means an overridden virtual function, and |void foo() final;| means an overridden virtual function that cannot be further overridden. All else equal, shorter is better. But this concision hurts readability, even past the non-obvious final/override = virtual implication others have noted. (And yes, C++ can/should permit final/override on non-virtuals. JSString and subclasses would be immediate users.) Requiring removal of virtual from the signature for final/override prevents simply examining a declaration's start to determine whether the function is virtual. You must read the entire declaration to know: a problem because final/override can blend in. For longer (especially multiline) declarations this matters. Consider these SpiderMonkey declarations: /* Standard internal methods. */ virtual bool getOwnPropertyDescriptor(JSContext* cx, HandleObject proxy, HandleId id, MutableHandleJSPropertyDescriptor desc) const override; virtual bool defineProperty(JSContext* cx, HandleObject proxy, HandleId id, HandleJSPropertyDescriptor desc, ObjectOpResult result) const override; virtual bool ownPropertyKeys(JSContext* cx, HandleObject proxy, AutoIdVector props) const override; virtual bool delete_(JSContext* cx, HandleObject proxy, HandleId id, ObjectOpResult result) const override; virtual is extraordinarily clear in starting each declaration. override and final alone would be obscured at the end of a long string of text, especially when skimming. (I disagree with assuming syntax coloring penalizing non-IDE users.) I think ideal style requires as many of virtual/override/final as apply. Virtual for easy reading. Override to be clear when it occurs. And final when that's what you want. (Re dholbert's subtle point: |virtual void foo() final| loses strictness, but -Woverloaded-virtual gives it back.) * It will allow us to remove NS_IMETHODIMP, and use NS_IMETHOD instead. A better alternative, that exposes standard C++ and macro-hides non-standard gunk, would be to use NS_IMETHOD for everything and directly use |virtual| in declarations. This point is no justification for changing style rules. Jeff ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
On Fri, May 01, 2015 at 05:05:09AM -0700, finnbry...@gmail.com wrote:
On Friday, May 1, 2015 at 12:03:35 PM UTC+1, Aryeh Gregor wrote:
On Thu, Apr 30, 2015 at 6:49 PM, Trevor Saunders wrote:
I don't think it would actually be backward incompatible the only
changes would be turning invalid programs into valid ones.
Given that void foo() override; currently makes foo() a virtual
function, allowing override on non-virtual functions would have to
change the meaning to make foo() non-virtual.
Not exactly, afaict, since it would also produce an error in any case where
foo wasn't *already* implicitly a virtual. So adding override can only add
additional information in the case where it causes an error. And since nobody
cares (from a general pov) about turning invalid programs into valid ones, it
could theoretically be changed in a future C++ version.
That said, it won't happen, since the entire purpose of override is to allow
programmers to explicitly confirm that they are overriding a virtual method
from a superclass, so allowing override for a method that doesn't override a
virtual is completely nonsensical. If it was valid for methods that didn't
override, it would act exactly the same as virtual, so would serve no
purpose, unless it counted as virtual only if overriding a virtual,
otherwise non-virtual, in which case it would be identical to the implicit
behavior, except it would cause huge confusion if applied to a non-overriding
method. So you can probably trust that will never happen.
there is no reason override has to mean that you are overriding a
virtual method as opposed to overriding a non virtual member. Consider
this program
struct Base
{
bool IsAFoo() { return mIsAFoo; }
bool mIsAFoo;
};
struct Foo
{
bool IsAFoo() { return true; }
};
One might wish to mark Foo::IsAFoo override so the compiler checks it
shadows Base::IsAFoo but both are still non virtual.
As for final, final attempts to prevent any subclasses from overriding a
virtual method. You can't override a non-virtual, so final on a non-virtual
would serve no purpose (except perhaps to prevent all subclasses from reusing
a method name and hiding the superclasses method? but that seems like
something that should be avoided regardless and is better solved with a
compiler-warning than a language feature...).
your exception here is correct, there's no reason final needs to be
specific to virtual overriding. There are good use cases for non
virtual overriding, so a compiler warning doesn't seem like a great idea
unless you have some sort of attribute to opt out maybe, at which point
why not put something in the standard so it works on all compilers?
Now, I don't think I care enough about this to write a proposal for it,
but I think it would be nice if the committee did it on its own.
Trev
___
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: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
Trevor One might wish to mark Foo::IsAFoo override so the compiler Trevor checks it shadows Base::IsAFoo but both are still non virtual. Yes, I agree, one might. But that doesn't really have any bearing on the present. Right now, override is defined as only making sense on virtual functions. And, I think we should only develop using the language we have -- not hypothetical future changes to the language. Plus, we have automated rewriting tools. If we're positing that C++ might change in this way, then I think it's equally valid to posit that the tools will also change appropriately. Tom ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
On Thu, Apr 30, 2015 at 6:49 PM, Trevor Saunders tbsau...@tbsaunde.org wrote: I don't think it would actually be backward incompatible the only changes would be turning invalid programs into valid ones. Given that void foo() override; currently makes foo() a virtual function, allowing override on non-virtual functions would have to change the meaning to make foo() non-virtual. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
On Wed, Apr 29, 2015 at 02:53:03PM -0400, Ehsan Akhgari wrote: On 2015-04-27 9:54 PM, Trevor Saunders wrote: On Mon, Apr 27, 2015 at 09:07:51PM -0400, Ehsan Akhgari wrote: On Mon, Apr 27, 2015 at 5:45 PM, Trevor Saunders tbsau...@tbsaunde.org wrote: On Mon, Apr 27, 2015 at 03:48:48PM -0400, Ehsan Akhgari wrote: Right now, our coding style requires that both the virtual and override keywords to be specified for overridden virtual functions. A few things have changed since we decided that a number of years ago: 1. The override and final keywords are now available on all of the compilers that we build with, and we have stopped supporting compilers that do not support these features. 2. We have very recently gained the ability to run clang-based mass source code transformations over our code that would let us enforce the coding style [1]. I would like to propose a change to our coding style, and run a mass transformation over our code in order to enforce it. I think we should change it to require the usage of exactly one of these keywords per *overridden* function: virtual, override, and final. Here are the advantages: I believe we have some cases in the tree where a virtual function doesn't override but is final so you need to write virtual final. I believe one of those cases may be so a function can be called indirectly from outside libxul, and another might be to prevent people using some cc macros incorrectly. Any chance you could point us to those functions, please? http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsCycleCollectionParticipant.h#548 Hmm, we can probably make an exception for this one. and this one isn't final, but we could make it final if the tu will be built into libxul (because then devirtualization is fine) http://mxr.mozilla.org/mozilla-central/source/dom/base/nsIDocument.h#1287 I'm not sure why that function is virtual. If it's just in order to make it possible to call it through a vtable from outside of libxul, I'm not sure why we need to treat it differently than other XPCOM APIs for example. If this is not used outside of libxul, we can probably make it non-virtual. its the former. We don't need to do anything special, but we could if we wanted. All that said it turns out its not terribly related. * It is a more succinct, as |virtual void foo() override;| doesn't convey more information than |void foo() override;|. personally I think it takes significantly more mental effort to read void foo() final; to mean it overrides something and is virtual than if its explicit as in virtual void foo() override final; and actually I'd also like it if C++ changed to allow override and final on non virtual functions in which case this would be a loss of information. Well, we're not talking about changing C++. ;-) But why do you find it I didn't say we were, but should such a change happen this would then be confusing. C++ doesn't usually change in backwards incompatible ways, and for all practical intents and purposes we can proceed under the assumption that this will never happen, so we don't need to protect against it. I don't think it would actually be backward incompatible the only changes would be turning invalid programs into valid ones. more clear to say |virtual ... final| than |... final|? They both convey the exact same amount of information. Is it just habit and/or personal preference? its explicit vs implicit yes it is true that you can derive foo is virtual from void foo() final; it doesn't take any habit or thinking to see that from virtual void foo() override final; but I would claim its not as obvious with void foo() final; I don't think that's really a preference more than say prefering text files to gziped files ;) I disagree. I understand the argument of someone not knowing these new keywords not understanding the distinction, but since we are already using these keywords, I think it is reasonable to expect people to either know or learn what these keywords mean. And once they do, then it really becomes a matter of preference. Another way to phrase this would be: if someone wonders whether foo in |void foo() final;| is virtual or not, they need to study the meaning of these keywords. :-) yes, but they also need to think about the meaning instead of just reading. * It can be easily enforced using clang-tidy across the entire code base. That doesn't really seem like an argument for choosing this particular style we could as easily check that virtual functions are always marked virtual, and marked override if possible. Except that is more tricky to get right. Please see bug 1158776. not if we have a static analysis that checks override is always present. We don't have that. Please note that I'm really not interested in building a whole set of infrastructure just in order to keep the current wording of the coding style. I
Re: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
On 2015-04-28 6:52 AM, Aryeh Gregor wrote: But why do you find it more clear to say |virtual ... final| than |... final|? They both convey the exact same amount of information. Is it just habit and/or personal preference? Personally, I was surprised when I first learned that override/final can only be used on virtual functions. I was definitely unaware that override/final imply virtual until I read this thread. It seems David Baron also didn't know. So I think keeping virtual makes things clearer for people who aren't C++ gurus like you. :) I would wager that most of our developers would not realize that void foo() override; is a virtual function, especially if they didn't think about it much. As I said, I do believe the argument of people being new to these C++11 keywords not knowing what they mean exactly, but the fact that they only apply to virtual functions usually appears in the first paragraph if not the first sentence of any documentation I've seen so far (for example, http://en.cppreference.com/w/cpp/language/override and http://en.cppreference.com/w/cpp/language/final), so I'd say it is pretty easy to learn and I do hope that this thread has at least helped with that a bit. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
On 2015-04-27 9:54 PM, Trevor Saunders wrote: On Mon, Apr 27, 2015 at 09:07:51PM -0400, Ehsan Akhgari wrote: On Mon, Apr 27, 2015 at 5:45 PM, Trevor Saunders tbsau...@tbsaunde.org wrote: On Mon, Apr 27, 2015 at 03:48:48PM -0400, Ehsan Akhgari wrote: Right now, our coding style requires that both the virtual and override keywords to be specified for overridden virtual functions. A few things have changed since we decided that a number of years ago: 1. The override and final keywords are now available on all of the compilers that we build with, and we have stopped supporting compilers that do not support these features. 2. We have very recently gained the ability to run clang-based mass source code transformations over our code that would let us enforce the coding style [1]. I would like to propose a change to our coding style, and run a mass transformation over our code in order to enforce it. I think we should change it to require the usage of exactly one of these keywords per *overridden* function: virtual, override, and final. Here are the advantages: I believe we have some cases in the tree where a virtual function doesn't override but is final so you need to write virtual final. I believe one of those cases may be so a function can be called indirectly from outside libxul, and another might be to prevent people using some cc macros incorrectly. Any chance you could point us to those functions, please? http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsCycleCollectionParticipant.h#548 Hmm, we can probably make an exception for this one. and this one isn't final, but we could make it final if the tu will be built into libxul (because then devirtualization is fine) http://mxr.mozilla.org/mozilla-central/source/dom/base/nsIDocument.h#1287 I'm not sure why that function is virtual. If it's just in order to make it possible to call it through a vtable from outside of libxul, I'm not sure why we need to treat it differently than other XPCOM APIs for example. If this is not used outside of libxul, we can probably make it non-virtual. * It is a more succinct, as |virtual void foo() override;| doesn't convey more information than |void foo() override;|. personally I think it takes significantly more mental effort to read void foo() final; to mean it overrides something and is virtual than if its explicit as in virtual void foo() override final; and actually I'd also like it if C++ changed to allow override and final on non virtual functions in which case this would be a loss of information. Well, we're not talking about changing C++. ;-) But why do you find it I didn't say we were, but should such a change happen this would then be confusing. C++ doesn't usually change in backwards incompatible ways, and for all practical intents and purposes we can proceed under the assumption that this will never happen, so we don't need to protect against it. more clear to say |virtual ... final| than |... final|? They both convey the exact same amount of information. Is it just habit and/or personal preference? its explicit vs implicit yes it is true that you can derive foo is virtual from void foo() final; it doesn't take any habit or thinking to see that from virtual void foo() override final; but I would claim its not as obvious with void foo() final; I don't think that's really a preference more than say prefering text files to gziped files ;) I disagree. I understand the argument of someone not knowing these new keywords not understanding the distinction, but since we are already using these keywords, I think it is reasonable to expect people to either know or learn what these keywords mean. And once they do, then it really becomes a matter of preference. Another way to phrase this would be: if someone wonders whether foo in |void foo() final;| is virtual or not, they need to study the meaning of these keywords. :-) * It can be easily enforced using clang-tidy across the entire code base. That doesn't really seem like an argument for choosing this particular style we could as easily check that virtual functions are always marked virtual, and marked override if possible. Except that is more tricky to get right. Please see bug 1158776. not if we have a static analysis that checks override is always present. We don't have that. Please note that I'm really not interested in building a whole set of infrastructure just in order to keep the current wording of the coding style. I agree that we could come up with ways of keeping the existing coding style, but this discussion is happening because I see no value in that work, and am arguing for changing that. So I'm more interested in arguments for why the current coding style is better, than how we can keep it working. * It makes it easier to determine what kind of function you are looking at by just looking at its declaration. |virtual void foo();| means a virtual
Re: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
I think this is a good plan, and the harmony with the Google style guide is a nice incidental benefit. Does this mean that every c++ *must* be marked with override, or is that still optional? Do we intend to make that a requirement in the future? --BDS On Mon, Apr 27, 2015 at 3:48 PM, Ehsan Akhgari ehsan.akhg...@gmail.com wrote: Right now, our coding style requires that both the virtual and override keywords to be specified for overridden virtual functions. A few things have changed since we decided that a number of years ago: 1. The override and final keywords are now available on all of the compilers that we build with, and we have stopped supporting compilers that do not support these features. 2. We have very recently gained the ability to run clang-based mass source code transformations over our code that would let us enforce the coding style [1]. I would like to propose a change to our coding style, and run a mass transformation over our code in order to enforce it. I think we should change it to require the usage of exactly one of these keywords per *overridden* function: virtual, override, and final. Here are the advantages: * It is a more succinct, as |virtual void foo() override;| doesn't convey more information than |void foo() override;|. * It can be easily enforced using clang-tidy across the entire code base. * It makes it easier to determine what kind of function you are looking at by just looking at its declaration. |virtual void foo();| means a virtual function that is not overridden, |void foo() override;| means an overridden virtual function, and |void foo() final;| means an overridden virtual function that cannot be further overridden. * It allows us to be in sync with the Google C++ Style on this issue [2]. * It will allow us to remove NS_IMETHODIMP, and use NS_IMETHOD instead. Please let me know what you think! [1] Facilitated by bug 904572. Also see bug 1158776 for my first attempt at this. [2] http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance Cheers, -- Ehsan -- Benjamin Smedberg Engineering Manager, Firefox ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
Ehsan Akhgari writes: On 2015-04-27 9:54 PM, Trevor Saunders wrote: On Mon, Apr 27, 2015 at 09:07:51PM -0400, Ehsan Akhgari wrote: On Mon, Apr 27, 2015 at 5:45 PM, Trevor Saunders tbsau...@tbsaunde.org wrote: I believe we have some cases in the tree where a virtual function doesn't override but is final so you need to write virtual final. I believe one of those cases may be so a function can be called indirectly from outside libxul, and another might be to prevent people using some cc macros incorrectly. Any chance you could point us to those functions, please? http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsCycleCollectionParticipant.h#548 Hmm, we can probably make an exception for this one. (I assume below that you are proposing handling these through method-specific exceptions.) and this one isn't final, but we could make it final if the tu will be built into libxul (because then devirtualization is fine) http://mxr.mozilla.org/mozilla-central/source/dom/base/nsIDocument.h#1287 I'm not sure why that function is virtual. If it's just in order to make it possible to call it through a vtable from outside of libxul, I'm not sure why we need to treat it differently than other XPCOM APIs for example. If this is not used outside of libxul, we can probably make it non-virtual. XPCOM APIs are usually abstract interfaces and so not final. final could be removed and have valid code, but may be a useful optimization or enforcement of intent. virtual final seems necessary for some use cases, and there is no redundancy. Is there a need to ban this usage? ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
Ehsan Akhgari writes: I think there's a typo of some sort in the question, but if you meant every overriding function must be marked with override, then yes, that is the change I'm proposing, but the good news is that you can now run clang-tidy on the entire tree and get it to rewrite the source code to make sure that the override keywords are present where they are needed, and we can do that as often as we would like. IOW, this can be done without requiring every C++ programmer to remember to do it always. I fear that an automatic update would be more than just enforcing a style because override keywords imply programmer intent. If the proposal is to periodically automatically add override keywords where methods override but are currently not annotated as such, then it seems we should we have an annotation to indicate that a method is not intended to override. However, that would require annotating all methods. This seems similar to the compiler warning situation. Usually at least, I don't think we should automatically modify the code in line with how the compiler reads the code just to silence the warning. Instead the warning is there to indicate that a programmer needs to check the code. Perhaps though there is a case for a one-off change to add override automatically so that warnings can be enabled on new code. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
On 2015-04-29 10:23 AM, Benjamin Smedberg wrote: I think this is a good plan, and the harmony with the Google style guide is a nice incidental benefit. Does this mean that every c++ *must* be marked with override, or is that still optional? Do we intend to make that a requirement in the future? I think there's a typo of some sort in the question, but if you meant every overriding function must be marked with override, then yes, that is the change I'm proposing, but the good news is that you can now run clang-tidy on the entire tree and get it to rewrite the source code to make sure that the override keywords are present where they are needed, and we can do that as often as we would like. IOW, this can be done without requiring every C++ programmer to remember to do it always. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
On Tue, Apr 28, 2015 at 4:07 AM, Ehsan Akhgari ehsan.akhg...@gmail.com wrote: Well, we're not talking about changing C++. ;-) My understanding is that the C++ committee will never change the meaning of existing programs without extremely compelling reason, so if override currently implies virtual, I think we can safely assume that will never be changed. But why do you find it more clear to say |virtual ... final| than |... final|? They both convey the exact same amount of information. Is it just habit and/or personal preference? Personally, I was surprised when I first learned that override/final can only be used on virtual functions. I was definitely unaware that override/final imply virtual until I read this thread. It seems David Baron also didn't know. So I think keeping virtual makes things clearer for people who aren't C++ gurus like you. :) I would wager that most of our developers would not realize that void foo() override; is a virtual function, especially if they didn't think about it much. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
On Tue, Apr 28, 2015 at 3:52 AM, Aryeh Gregor a...@aryeh.name wrote: Personally, I was surprised when I first learned that override/final can only be used on virtual functions. I was definitely unaware that override/final imply virtual until I read this thread. It seems David Baron also didn't know. So I think keeping virtual makes things clearer for people who aren't C++ gurus like you. :) I would wager that most of our developers would not realize that void foo() override; is a virtual function, especially if they didn't think about it much. It's not that much of a stretch to imagine that override and final only make sense with virtual functions. I would like exactly one permissible syntax. The one that Ehsan has proposed is the cleanest. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
On 04/27/2015 12:48 PM, Ehsan Akhgari wrote: I think we should change it to require the usage of exactly one of these keywords per *overridden* function: virtual, override, and final. Let me attempt to clarify why the exactly one requirement here is important. (It's non-obvious.) This verbose function-signature... virtual void foo() final override; ...can be expressed as the following, without removing any strictness: void foo() final BUT, if we add back the virtual keyword, we *lose some strictness*. In other words, this is *not* equivalent to the versions above: virtual void foo() final Unless the formulations above, this version isn't guaranteed to override. The keywords final override can be viewed as redundant, but only if the virtual keyword is not present. This is because final implies virtual, and virtualness-without-an-explicit-virtual-keyword implies override. So final + lack-of-virtual-keyword implies override. Hence the exactly one requirement that ehsan is proposing (which Google has adopted). * It makes it easier to determine what kind of function you are looking at by just looking at its declaration. |virtual void foo();| means a virtual function that is not overridden, |void foo() override;| means an overridden virtual function, and |void foo() final;| means an overridden virtual function that cannot be further overridden. After discussing briefly with ehsan on IRC -- let me clarify the language on this, since the word overridden is ambiguous here. (ehsan was mostly using it to mean is an override of something else) I'll restate the three categories, with less ambiguous language: - |virtual void foo();| means a virtual function that is not an override of a superclass method. - |void foo() override;| means an overriding virtual function. - |void foo() final;| means an overriding virtual function that cannot be further overridden. * It will allow us to remove NS_IMETHODIMP, and use NS_IMETHOD instead. (FWIW: This is because NS_METHODIMP expands to the exact same thing as NS_IMETHOD, minus the 'virtual' keyword: https://mxr.mozilla.org/mozilla-central/source/xpcom/base/nscore.h?rev=7e4e5e971d95mark=96-97#96 So if we were to drop 'virtual' from NS_IMETHOD -- as we probably would want to if we adopt ehsan's proposal -- then NS_IMETHODIMP would have no reason to exist.) ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
I completely support this. Let the needless verbosity end! - Seth On Apr 27, 2015, at 12:48 PM, Ehsan Akhgari ehsan.akhg...@gmail.com wrote: Right now, our coding style requires that both the virtual and override keywords to be specified for overridden virtual functions. A few things have changed since we decided that a number of years ago: 1. The override and final keywords are now available on all of the compilers that we build with, and we have stopped supporting compilers that do not support these features. 2. We have very recently gained the ability to run clang-based mass source code transformations over our code that would let us enforce the coding style [1]. I would like to propose a change to our coding style, and run a mass transformation over our code in order to enforce it. I think we should change it to require the usage of exactly one of these keywords per *overridden* function: virtual, override, and final. Here are the advantages: * It is a more succinct, as |virtual void foo() override;| doesn't convey more information than |void foo() override;|. * It can be easily enforced using clang-tidy across the entire code base. * It makes it easier to determine what kind of function you are looking at by just looking at its declaration. |virtual void foo();| means a virtual function that is not overridden, |void foo() override;| means an overridden virtual function, and |void foo() final;| means an overridden virtual function that cannot be further overridden. * It allows us to be in sync with the Google C++ Style on this issue [2]. * It will allow us to remove NS_IMETHODIMP, and use NS_IMETHOD instead. Please let me know what you think! [1] Facilitated by bug 904572. Also see bug 1158776 for my first attempt at this. [2] http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance Cheers, -- 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
Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
Right now, our coding style requires that both the virtual and override keywords to be specified for overridden virtual functions. A few things have changed since we decided that a number of years ago: 1. The override and final keywords are now available on all of the compilers that we build with, and we have stopped supporting compilers that do not support these features. 2. We have very recently gained the ability to run clang-based mass source code transformations over our code that would let us enforce the coding style [1]. I would like to propose a change to our coding style, and run a mass transformation over our code in order to enforce it. I think we should change it to require the usage of exactly one of these keywords per *overridden* function: virtual, override, and final. Here are the advantages: * It is a more succinct, as |virtual void foo() override;| doesn't convey more information than |void foo() override;|. * It can be easily enforced using clang-tidy across the entire code base. * It makes it easier to determine what kind of function you are looking at by just looking at its declaration. |virtual void foo();| means a virtual function that is not overridden, |void foo() override;| means an overridden virtual function, and |void foo() final;| means an overridden virtual function that cannot be further overridden. * It allows us to be in sync with the Google C++ Style on this issue [2]. * It will allow us to remove NS_IMETHODIMP, and use NS_IMETHOD instead. Please let me know what you think! [1] Facilitated by bug 904572. Also see bug 1158776 for my first attempt at this. [2] http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance Cheers, -- Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
On Mon, Apr 27, 2015 at 03:48:48PM -0400, Ehsan Akhgari wrote: Right now, our coding style requires that both the virtual and override keywords to be specified for overridden virtual functions. A few things have changed since we decided that a number of years ago: 1. The override and final keywords are now available on all of the compilers that we build with, and we have stopped supporting compilers that do not support these features. 2. We have very recently gained the ability to run clang-based mass source code transformations over our code that would let us enforce the coding style [1]. I would like to propose a change to our coding style, and run a mass transformation over our code in order to enforce it. I think we should change it to require the usage of exactly one of these keywords per *overridden* function: virtual, override, and final. Here are the advantages: I believe we have some cases in the tree where a virtual function doesn't override but is final so you need to write virtual final. I believe one of those cases may be so a function can be called indirectly from outside libxul, and another might be to prevent people using some cc macros incorrectly. * It is a more succinct, as |virtual void foo() override;| doesn't convey more information than |void foo() override;|. personally I think it takes significantly more mental effort to read void foo() final; to mean it overrides something and is virtual than if its explicit as in virtual void foo() override final; and actually I'd also like it if C++ changed to allow override and final on non virtual functions in which case this would be a loss of information. * It can be easily enforced using clang-tidy across the entire code base. That doesn't really seem like an argument for choosing this particular style we could as easily check that virtual functions are always marked virtual, and marked override if possible. * It makes it easier to determine what kind of function you are looking at by just looking at its declaration. |virtual void foo();| means a virtual function that is not overridden, |void foo() override;| means an overridden virtual function, and |void foo() final;| means an overridden virtual function that cannot be further overridden. this seems to be more an advantage of any enforced rule. That is if we just enforced that any function that overrides is marked override then you would know that virtual void foo(); doesn't override, and otherwise override would always be present which would make it clear it does override in addition to possibly being final. * It allows us to be in sync with the Google C++ Style on this issue [2]. I don't personally care about that much. * It will allow us to remove NS_IMETHODIMP, and use NS_IMETHOD instead. That doesn't really seem like much of an improvement, and of course really we should just get rid of both but that's more work. Trev Please let me know what you think! [1] Facilitated by bug 904572. Also see bug 1158776 for my first attempt at this. [2] http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance Cheers, -- 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: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
That's correct. Using override and final on a non-virtual function is a compile time error. On Apr 27, 2015 5:34 PM, L. David Baron dba...@dbaron.org wrote: On Monday 2015-04-27 15:48 -0400, Ehsan Akhgari wrote: Right now, our coding style requires that both the virtual and override keywords to be specified for overridden virtual functions. A few things have changed since we decided that a number of years ago: 1. The override and final keywords are now available on all of the compilers that we build with, and we have stopped supporting compilers that do not support these features. 2. We have very recently gained the ability to run clang-based mass source code transformations over our code that would let us enforce the coding style [1]. I would like to propose a change to our coding style, and run a mass transformation over our code in order to enforce it. I think we should change it to require the usage of exactly one of these keywords per *overridden* function: virtual, override, and final. Here are the advantages: Are the override and final keywords not allowed on non-virtual functions? -David -- 턞 L. David Baron http://dbaron.org/ 턂 턢 Mozilla https://www.mozilla.org/ 턂 Before I built a wall I'd ask to know What I was walling in or walling out, And to whom I was like to give offense. - Robert Frost, Mending Wall (1914) ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
On Mon, Apr 27, 2015 at 5:45 PM, Trevor Saunders tbsau...@tbsaunde.org wrote: On Mon, Apr 27, 2015 at 03:48:48PM -0400, Ehsan Akhgari wrote: Right now, our coding style requires that both the virtual and override keywords to be specified for overridden virtual functions. A few things have changed since we decided that a number of years ago: 1. The override and final keywords are now available on all of the compilers that we build with, and we have stopped supporting compilers that do not support these features. 2. We have very recently gained the ability to run clang-based mass source code transformations over our code that would let us enforce the coding style [1]. I would like to propose a change to our coding style, and run a mass transformation over our code in order to enforce it. I think we should change it to require the usage of exactly one of these keywords per *overridden* function: virtual, override, and final. Here are the advantages: I believe we have some cases in the tree where a virtual function doesn't override but is final so you need to write virtual final. I believe one of those cases may be so a function can be called indirectly from outside libxul, and another might be to prevent people using some cc macros incorrectly. Any chance you could point us to those functions, please? * It is a more succinct, as |virtual void foo() override;| doesn't convey more information than |void foo() override;|. personally I think it takes significantly more mental effort to read void foo() final; to mean it overrides something and is virtual than if its explicit as in virtual void foo() override final; and actually I'd also like it if C++ changed to allow override and final on non virtual functions in which case this would be a loss of information. Well, we're not talking about changing C++. ;-) But why do you find it more clear to say |virtual ... final| than |... final|? They both convey the exact same amount of information. Is it just habit and/or personal preference? * It can be easily enforced using clang-tidy across the entire code base. That doesn't really seem like an argument for choosing this particular style we could as easily check that virtual functions are always marked virtual, and marked override if possible. Except that is more tricky to get right. Please see bug 1158776. * It makes it easier to determine what kind of function you are looking at by just looking at its declaration. |virtual void foo();| means a virtual function that is not overridden, |void foo() override;| means an overridden virtual function, and |void foo() final;| means an overridden virtual function that cannot be further overridden. this seems to be more an advantage of any enforced rule. That is if we just enforced that any function that overrides is marked override then you would know that virtual void foo(); doesn't override, and otherwise override would always be present which would make it clear it does override in addition to possibly being final. Yes, but again the point is that 1) one alternative repeats redundant keywords, and 2) we don't *need* to use the virtual keyword on overriding functions any more. Perhaps the second point is not clear from my first email. Before, because not all of the compilers we target supported override and final, we *needed* to keep the virtual function, but now we don't, so using virtual for overriding function now requires a justification, contrary to our past practice. * It allows us to be in sync with the Google C++ Style on this issue [2]. I don't personally care about that much. The reason why this is important is that many of the existing tools for massive rewriting of code bases have been written with that coding style in mind, so not diverging from that style enables us to use those tools out of the box. (But this is just a nicety, of course.) * It will allow us to remove NS_IMETHODIMP, and use NS_IMETHOD instead. That doesn't really seem like much of an improvement, and of course really we should just get rid of both but that's more work. I don't understand how this is not an improvement. I have seen *tons* of places where people are not sure which one they are supposed to use, or use the wrong one (for example, NS_IMETHODIMP for inline functions inside class bodies -- thankfully the virtual keyword is redundant. ;-) Also I don't understand why you think we should get rid of both. Not meaning to open a discussion on that since that's off-topic, but we should definitely not get rid of this macro, since it has a job that it's really good at (encapsulating the COM compatible calling convention on Windows.) Perhaps you meant taking the nsresult out of it, but that just gives us more verbosity which is not nice.. But I digress! Please let me know what you think! [1] Facilitated by bug 904572. Also see bug 1158776 for my first
Re: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used
On Mon, Apr 27, 2015 at 09:07:51PM -0400, Ehsan Akhgari wrote:
On Mon, Apr 27, 2015 at 5:45 PM, Trevor Saunders tbsau...@tbsaunde.org
wrote:
On Mon, Apr 27, 2015 at 03:48:48PM -0400, Ehsan Akhgari wrote:
Right now, our coding style requires that both the virtual and override
keywords to be specified for overridden virtual functions. A few things
have changed since we decided that a number of years ago:
1. The override and final keywords are now available on all of the
compilers that we build with, and we have stopped supporting compilers
that
do not support these features.
2. We have very recently gained the ability to run clang-based mass
source
code transformations over our code that would let us enforce the coding
style [1].
I would like to propose a change to our coding style, and run a mass
transformation over our code in order to enforce it. I think we should
change it to require the usage of exactly one of these keywords per
*overridden* function: virtual, override, and final. Here are the
advantages:
I believe we have some cases in the tree where a virtual function
doesn't override but is final so you need to write virtual final. I
believe one of those cases may be so a function can be called indirectly
from outside libxul, and another might be to prevent people using some
cc macros incorrectly.
Any chance you could point us to those functions, please?
http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsCycleCollectionParticipant.h#548
and this one isn't final, but we could make it final if the tu will be
built into libxul (because then devirtualization is fine)
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsIDocument.h#1287
* It is a more succinct, as |virtual void foo() override;| doesn't convey
more information than |void foo() override;|.
personally I think it takes significantly more mental effort to read
void foo() final; to mean it overrides something and is virtual than if
its explicit as in virtual void foo() override final;
and actually I'd also like it if C++ changed to allow override and final
on non virtual functions in which case this would be a loss of
information.
Well, we're not talking about changing C++. ;-) But why do you find it
I didn't say we were, but should such a change happen this would then be
confusing.
more clear to say |virtual ... final| than |... final|? They both convey
the exact same amount of information. Is it just habit and/or personal
preference?
its explicit vs implicit yes it is true that you can derive foo is
virtual from void foo() final; it doesn't take any habit or thinking to
see that from virtual void foo() override final; but I would claim its
not as obvious with void foo() final; I don't think that's really a
preference more than say prefering text files to gziped files ;)
* It can be easily enforced using clang-tidy across the entire code base.
That doesn't really seem like an argument for choosing this particular
style we could as easily check that virtual functions are always marked
virtual, and marked override if possible.
Except that is more tricky to get right. Please see bug 1158776.
not if we have a static analysis that checks override is always present.
* It makes it easier to determine what kind of function you are looking
at
by just looking at its declaration. |virtual void foo();| means a
virtual
function that is not overridden, |void foo() override;| means an
overridden
virtual function, and |void foo() final;| means an overridden virtual
function that cannot be further overridden.
this seems to be more an advantage of any enforced rule. That is if we
just enforced that any function that overrides is marked override then
you would know that virtual void foo(); doesn't override, and otherwise
override would always be present which would make it clear it does
override in addition to possibly being final.
Yes, but again the point is that 1) one alternative repeats redundant
sure, they're redundant if your only goal is to wwrite the shortest
possibleC++, but I'd claim if your goal is to write readable code they
are not redundant which is basically my whole point here.
keywords, and 2) we don't *need* to use the virtual keyword on overriding
functions any more. Perhaps the second point is not clear from my first
email. Before, because not all of the compilers we target supported
override and final, we *needed* to keep the virtual function, but now we
no, we never *needed* to use virtual on overides that is
class a { virtual void foo(); };
class b : a
{
void foo();
};
is perfectly valid C++.
don't, so using virtual for overriding function now requires a
justification, contrary to our past practice.
* It allows us to be in sync with the Google C++ Style on this issue [2].
I don't personally care about that much.
