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