Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used

[Ehsan Akhgari]

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

[Seth Fowler]

I completely support this. Let the needless verbosity end!

- Seth

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

[Daniel Holbert]

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=7e4e5e971d95&mark=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.)

[Daniel Holbert]

On 04/27/2015 01:47 PM, Daniel Holbert wrote:
> 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.

(er s/Unless/Unlike/, of course.)

[Trevor Saunders]

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

[Ehsan Akhgari]

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)
>

[Ehsan Akhgari]

On Mon, Apr 27, 2015 at 5:45 PM, Trevor Saunders <tbsa...@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 attempt
> > at this.
> > [2]
> >
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance
> >
> > Cheers,
> > --
> > Ehsan
> > _______________________________________________
> > dev-platform mailing list
> > dev-pl...@lists.mozilla.org
> > https://lists.mozilla.org/listinfo/dev-platform
> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>

--
Ehsan

[Trevor Saunders]

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.
> >
>
> 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. ;-)

Sure, it is a improvement, but the code implementing xpcom interfaces
over all probably isn't changing that much, and the total amount of such
code is slowely going down. So it'll probably improve embedding/ a
bit, but that code will still be very unpleasent to read.

> 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!

totally off topic, but no, I meant we should get rid of the COM
compatibility on windows and hence the need for the macro.

Trev

[Aryeh Gregor]

On Tue, Apr 28, 2015 at 4:07 AM, Ehsan Akhgari <ehsan....@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.

[Martin Thomson]

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.

[Benjamin Smedberg]

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

[Ehsan Akhgari]

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.

[Ehsan Akhgari]

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 <tbsa...@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
>>>> 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.

The redundancy here hurts the readability of the code, because our
brains will need to process what they read. I'm explicitly not
interested in optimizing for writing the shortest code.

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

Yes, but that would make it impossible to tell whether a method is
virtual by just looking at its declaration, which hurts the readability
of the code. That's why we needed to use the virtual keyword before we
could rely on the existence of the override keyword.

>> 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. ;-)
>
> Sure, it is a improvement, but the code implementing xpcom interfaces
> over all probably isn't changing that much, and the total amount of such
> code is slowely going down. So it'll probably improve embedding/ a
> bit, but that code will still be very unpleasent to read.

That's not really true. We have a lot of XPCOM APIs that people use all
the time, for example, nsIRunnable, nsIObserver, etc.


Cheers,
Ehsan

[Ehsan Akhgari]

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.

[Karl Tomlinson]

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 <tbsa...@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?

[Karl Tomlinson]

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.

[Trevor Saunders]

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 <tbsa...@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 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.

I think it'd be pretty trivial to add this check, the compiler already
does all the hard parts for you. My point was that tooling isn't a
great reason to make either choice.

> >>>>* 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.
>
> The redundancy here hurts the readability of the code, because our brains
> will need to process what they read. I'm explicitly not interested in
> optimizing for writing the shortest code.

I'd say it increases readability because while you read more you need to
think about what you read less, but I guess we're now at the point
that's just a diffrence in how our brains work ;)

> >>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++.
>
> Yes, but that would make it impossible to tell whether a method is virtual
> by just looking at its declaration, which hurts the readability of the code.

yes, it would certainly make things unpleasent, but that's not what the
word "need" means ;-)

> That's why we needed to use the virtual keyword before we could rely on the
> existence of the override keyword.
>
> >>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. ;-)
> >
> >Sure, it is a improvement, but the code implementing xpcom interfaces
> >over all probably isn't changing that much, and the total amount of such
> >code is slowely going down. So it'll probably improve embedding/ a
> >bit, but that code will still be very unpleasent to read.
>
> That's not really true. We have a lot of XPCOM APIs that people use all the
> time, for example, nsIRunnable, nsIObserver, etc.

I guess it depends a lot what you work on. ersonally I don't remember
the last time I wrote NS_IMETHOD[,IMP].

Trev

>
>
> Cheers,
> Ehsan

[Aryeh Gregor]

On Thu, Apr 30, 2015 at 6:49 PM, Trevor Saunders <tbsa...@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.

[finnb...@gmail.com]

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.

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

[Trevor Saunders]

On Fri, May 01, 2015 at 05:05:09AM -0700, finnb...@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

[Tom Tromey]

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

[Jeff Walden]

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,
> MutableHandle<JSPropertyDescriptor> desc) const override;
> virtual bool defineProperty(JSContext* cx, HandleObject proxy, HandleId id,
> Handle<JSPropertyDescriptor> 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

[Ehsan Akhgari]

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. :-)

[Ehsan Akhgari]

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.

[Ehsan Akhgari]

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,
>> MutableHandle<JSPropertyDescriptor> desc) const override;
>> virtual bool defineProperty(JSContext* cx, HandleObject proxy, HandleId id,
>> Handle<JSPropertyDescriptor> 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. :-)

[Eric Shepherd (Sheppy)]

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....@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,
>>>
>>> MutableHandle<JSPropertyDescriptor> desc) const override;
>>> virtual bool defineProperty(JSContext* cx, HandleObject proxy,
>>> HandleId id,
>>> Handle<JSPropertyDescriptor> 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-pl...@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

[Karl Tomlinson]

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.

[Mike Hommey]

It *is* enabled and fatal, but there are a few places that disable it.

Mike

[Ehsan Akhgari]

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.

[Karl Tomlinson]

Ah, yes. Sorry to confusing things.

[Daniel Holbert]

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>

[Benjamin Smedberg]

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....@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-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>



--

[Ehsan Akhgari]

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-US&to=802537&from=802167>

Thanks,
Ehsan

[Gerald Squelart]

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?