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