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