Re: PSA: C++ virtual function declarations should specify only one of virtual, final, or override

[Chris Peterson]

On 2018-02-16 12:54 PM, Ben Kelly wrote:

Are we supposed to just use override or final on methods that are overriden
when the class itself is marked final?

Personally writing final again seems redundant with the class level final
and the override keyword seems more informative.


You could use either final or override. My lint just checks syntax 
(using a regular expression). It doesn't know whether the class is final 
or whether a function declaration without any virtual/final/override 
specifier is overriding a base class's virtual function. (A clang plugin 
could.)


Specifying final might be safer than override. Someone might later make 
the class not final and then the `override` functions in the class can 
now be overridden. "final" is also shorter than "override". :)


final implies override *if* the function declaration does not also 
specify virtual. You can't tell whether `virtual void Huh() final` is 
overriding a base class function Huh() or actually declaring a new 
"virtual" function that can never be overridden. I found a few instances 
of these virtual/final declarations in mozilla-central. That's why the 
style guide recommends never using more than one of virtual, final, or 
override.


___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: PSA: C++ virtual function declarations should specify only one of virtual, final, or override

[Chris Peterson]

On 2018-02-16 1:07 PM, L. David Baron wrote:

   virtual void Bad1() final

I think there might be some legitimate use cases for this one, when
a function needs to be virtual because it's required for the calling
convention (as part of a binary plugin API or binary embedding API,
for example), but it's also not to be overridden, and it's also not
overriding a virtual function on a base class.  While we've moved
away from binary plugin interfaces, I could imagine it the
definition of an API for embedding Gecko or some part of it.


That's an interesting point and there are some possible workarounds (below).

btw, I found four such non-overriding virtual/final declarations in 
mozilla-central (links below) while writing this lint script:


1. NOT_INHERITED_CANT_OVERRIDE is a debug macro that declares a 
non-overriding final virtual function (BaseCycleCollectable) as a 
compile-time check of some properties of CC-able classes.


2. AccessibleNode::GetParentObject(). GetParentObject() is a common 
virtual function in many DOM classes, but AccessibleNode does not derive 
from any base classes the define GetParentObject(). I think this might 
be some code that was copy/pasted. It doesn't need to be virtual.


3. WebCryptoTask::CalculateResult() and CallCallback() mirror virtual 
function names in the CryptoTask class, though WebCryptoTask does not 
actually derive from CryptoTask. These classes used to share code but 
were decoupled (in bug 1001691) so WebCryptoTask could be used on worker 
threads. These functions don't need to be virtual.


4. nsWindowBase::GetWindowHandle(), which does not override any base 
class functions. The only other function named GetWindowHandle() is 
MouseScrollHandler::EventInfo::GetWindowHandle() in an unrelated class.




I think it's reasonable to warn for it since the above case should
be pretty rare, but I'd be a little concerned about forbidding it
completely.


My lint check is currently written to run on checkins, so its warning 
would be treated as an error on Treeherder. Possible workarounds:


* Individual files or directories to be whitelisted in the lint script. 
This is easy.


* The virtual and final keywords could be moved to different lines. My 
lint is just a complicated regular expression and can't analyze virtual 
function declarations that span multiple lines.



[1] 
https://searchfox.org/mozilla-central/rev/a5abf843f8fac8530aa5de6fb40e16547bb4a47a/xpcom/base/nsCycleCollectionParticipant.h#619-629


[2] 
https://searchfox.org/mozilla-central/rev/a5abf843f8fac8530aa5de6fb40e16547bb4a47a/accessible/aom/AccessibleNode.h#37


[3] 
https://searchfox.org/mozilla-central/rev/a5abf843f8fac8530aa5de6fb40e16547bb4a47a/dom/crypto/WebCryptoTask.h#182,184


[4] 
https://searchfox.org/mozilla-central/rev/cac28623a15ace458a8f4526e107a71db1519daf/widget/windows/WinMouseScrollHandler.h#191

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: PSA: C++ virtual function declarations should specify only one of virtual, final, or override

[L. David Baron]

On Friday 2018-02-16 12:36 -0800, Chris Peterson wrote:
> Mozilla's C++ style guide [1] says (since 2015) virtual function
> declarations should specify only one of `virtual`, `final`, or `override`.
> 
> Over the weekend, I will land a mach lint check (bug 1436263) that will warn
> about some virtual style violations such as:
> 
>   virtual void Bad1() final

I think there might be some legitimate use cases for this one, when
a function needs to be virtual because it's required for the calling
convention (as part of a binary plugin API or binary embedding API,
for example), but it's also not to be overridden, and it's also not
overriding a virtual function on a base class.  While we've moved
away from binary plugin interfaces, I could imagine it the
definition of an API for embedding Gecko or some part of it.

I think it's reasonable to warn for it since the above case should
be pretty rare, but I'd be a little concerned about forbidding it
completely.

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


signature.asc
Description: PGP signature
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: PSA: C++ virtual function declarations should specify only one of virtual, final, or override

[Ben Kelly]

Are we supposed to just use override or final on methods that are overriden
when the class itself is marked final?

Personally writing final again seems redundant with the class level final
and the override keyword seems more informative.

On Fri, Feb 16, 2018 at 3:36 PM, Chris Peterson 
wrote:

> Mozilla's C++ style guide [1] says (since 2015) virtual function
> declarations should specify only one of `virtual`, `final`, or `override`.
>
> Over the weekend, I will land a mach lint check (bug 1436263) that will
> warn about some virtual style violations such as:
>
>   virtual void Bad1() final
>   void Bad2() final override
>   void Bad3() override final
>
> It won't warn about the redundant `override` in `virtual void NotBad()
> override` at this time because there are 8000+ instances in
> mozilla-central. Also, virtual/override is more of a style issue whereas
> virtual/final can mask real bugs.
>
> A clang-tidy check would be more thorough, but this mach lint is better
> than nothing until someone steps up to write a proper clang plugin. :) We
> had a clang-tidy check but it was disabled recently (bug 1420366) because
> it was too noisy (because it analyzed the post-processed source after
> NS_IMETHOD macros had been expanded to `virtual`).
>
>
> [1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_g
> uide/Coding_Style
> ___
> 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

PSA: C++ virtual function declarations should specify only one of virtual, final, or override

[Chris Peterson]

Mozilla's C++ style guide [1] says (since 2015) virtual function 
declarations should specify only one of `virtual`, `final`, or `override`.


Over the weekend, I will land a mach lint check (bug 1436263) that will 
warn about some virtual style violations such as:


  virtual void Bad1() final
  void Bad2() final override
  void Bad3() override final

It won't warn about the redundant `override` in `virtual void NotBad() 
override` at this time because there are 8000+ instances in 
mozilla-central. Also, virtual/override is more of a style issue whereas 
virtual/final can mask real bugs.


A clang-tidy check would be more thorough, but this mach lint is better 
than nothing until someone steps up to write a proper clang plugin. :) 
We had a clang-tidy check but it was disabled recently (bug 1420366) 
because it was too noisy (because it analyzed the post-processed source 
after NS_IMETHOD macros had been expanded to `virtual`).



[1] 
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform