On Mon, Apr 27, 2015 at 5:45 PM, Trevor Saunders <tbsau...@tbsaunde.org> wrote:
> On Mon, Apr 27, 2015 at 03:48:48PM -0400, Ehsan Akhgari wrote: > > Right now, our coding style requires that both the virtual and override > > keywords to be specified for overridden virtual functions. A few things > > have changed since we decided that a number of years ago: > > > > 1. The override and final keywords are now available on all of the > > compilers that we build with, and we have stopped supporting compilers > that > > do not support these features. > > 2. We have very recently gained the ability to run clang-based mass > source > > code transformations over our code that would let us enforce the coding > > style [1]. > > > > I would like to propose a change to our coding style, and run a mass > > transformation over our code in order to enforce it. I think we should > > change it to require the usage of exactly one of these keywords per > > *overridden* function: virtual, override, and final. Here are the > > advantages: > > I believe we have some cases in the tree where a virtual function > doesn't override but is final so you need to write virtual final. I > believe one of those cases may be so a function can be called indirectly > from outside libxul, and another might be to prevent people using some > cc macros incorrectly. > Any chance you could point us to those functions, please? > > * It is a more succinct, as |virtual void foo() override;| doesn't convey > > more information than |void foo() override;|. > > personally I think it takes significantly more mental effort to read > void foo() final; to mean it overrides something and is virtual than if > its explicit as in virtual void foo() override final; > > and actually I'd also like it if C++ changed to allow override and final > on non virtual functions in which case this would be a loss of > information. > Well, we're not talking about changing C++. ;-) But why do you find it more clear to say |virtual ... final| than |... final|? They both convey the exact same amount of information. Is it just habit and/or personal preference? > > * It can be easily enforced using clang-tidy across the entire code base. > > That doesn't really seem like an argument for choosing this particular > style we could as easily check that virtual functions are always marked > virtual, and marked override if possible. > Except that is more tricky to get right. Please see bug 1158776. > > * It makes it easier to determine what kind of function you are looking > at > > by just looking at its declaration. |virtual void foo();| means a > virtual > > function that is not overridden, |void foo() override;| means an > overridden > > virtual function, and |void foo() final;| means an overridden virtual > > function that cannot be further overridden. > > this seems to be more an advantage of any enforced rule. That is if we > just enforced that any function that overrides is marked override then > you would know that virtual void foo(); doesn't override, and otherwise > override would always be present which would make it clear it does > override in addition to possibly being final. > Yes, but again the point is that 1) one alternative repeats redundant keywords, and 2) we don't *need* to use the virtual keyword on overriding functions any more. Perhaps the second point is not clear from my first email. Before, because not all of the compilers we target supported override and final, we *needed* to keep the virtual function, but now we don't, so using virtual for overriding function now requires a justification, contrary to our past practice. > > * It allows us to be in sync with the Google C++ Style on this issue [2]. > > I don't personally care about that much. > The reason why this is important is that many of the existing tools for massive rewriting of code bases have been written with that coding style in mind, so not diverging from that style enables us to use those tools out of the box. (But this is just a nicety, of course.) > > * It will allow us to remove NS_IMETHODIMP, and use NS_IMETHOD instead. > > That doesn't really seem like much of an improvement, and of course > really we should just get rid of both but that's more work. > I don't understand how this is not an improvement. I have seen *tons* of places where people are not sure which one they are supposed to use, or use the "wrong" one (for example, NS_IMETHODIMP for inline functions inside class bodies -- thankfully the virtual keyword is redundant. ;-) Also I don't understand why you think we should get rid of both. Not meaning to open a discussion on that since that's off-topic, but we should definitely not get rid of this macro, since it has a job that it's really good at (encapsulating the COM compatible calling convention on Windows.) Perhaps you meant taking the nsresult out of it, but that just gives us more verbosity which is not nice.. But I digress! > > > > Please let me know what you think! > > > > [1] Facilitated by bug 904572. Also see bug 1158776 for my first 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
