Closed Bug 1436263 Opened 2 years ago Closed 2 years ago

Replace `final override` virtual function specifiers with just `final`

Categories

(Core :: General, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

Details

Attachments

(6 files)

The `override` in `final override` (and `override final`) is redundant because `final` is only legal on overridden virtual functions. These patches bring the code up to date with Mozilla's coding style guide, which specifies:

"Method declarations must use at most one of the following keywords: virtual, override, or final."

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

clang's -Winconsistent-missing-override warning recognizes that `final` implies `override`, but gcc's -Wsuggest-override warning does not and insists that `final` virtual functions also specify `override`. That's part of the reason those gcc's -Wsuggest-override warnings were not enabled in bug 1430669).

Here is a green Try build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ac9639b5116d913dfb485662a9d2cec246c1cf0
And we should add static analysis rule. It is particularly important to prevent `virtual foo() final;`.
Comment on attachment 8948914 [details]
Bug 1436263 - Part 1: Replace `final override` virtual function specifiers with just `final`.

https://reviewboard.mozilla.org/r/218336/#review224142

::: devtools/shared/heapsnapshot/HeapSnapshot.cpp:1258
(Diff revision 1)
>             oneByteStringsAlreadySerialized.init();
>    }
>  
>    ~StreamWriter() override { }
>  
> -  virtual bool writeMetadata(uint64_t timestamp) final override {
> +  virtual bool writeMetadata(uint64_t timestamp) final {

`virtual` must be removed (same for throughout the patch).
Comment on attachment 8948915 [details]
Bug 1436263 - Part 2: Replace `override final` virtual function specifiers with just `final`.

https://reviewboard.mozilla.org/r/218338/#review224218

Thanks!
Attachment #8948915 - Flags: review?(nfroyd) → review+
Comment on attachment 8948914 [details]
Bug 1436263 - Part 1: Replace `final override` virtual function specifiers with just `final`.

https://reviewboard.mozilla.org/r/218336/#review224212

Kimura-san raises an excellent point, but for cases where we have `virtual final override` functions, it seems preferable to me--at least for review purposes--to eliminate one keyword per commit.  Limiting things in this way makes the patch significantly easier to review.  It might be preferable to squash such patches before committing them; I can go either way.
Attachment #8948914 - Flags: review?(nfroyd) → review+
(In reply to Masatoshi Kimura [:emk] from comment #3)
> And we should add static analysis rule. It is particularly important to
> prevent `virtual foo() final;`.

I wrote a simple check for mach lint that can warn about redundant `final override`, but breaking the build for a `final override` seemed low value because it's not a functional bug.

I also wrote a mach lint check that can warn about function declarations that include more than one of `virtual`, `final`, or `override`. This check, however, is very noisy. The existing warnings (in 800+ source files) can be suppressed, but that limits the effectiveness of the lint.

I can submit either of these lint checks, if people think they are worthwhile.
Note that `final` implies `override` only if `virtual` is not present in the same declaration. At least this case must be warned if we do not change the current style guide.
So it will make the code more error-prone to change `virtual final override` to `virtual final`. Please remove `virtual` along with `override` although it does not have to be in the same changeset as Nathan said.
(In reply to Masatoshi Kimura [:emk] from comment #9)
> So it will make the code more error-prone to change `virtual final override`
> to `virtual final`. Please remove `virtual` along with `override` although
> it does not have to be in the same changeset as Nathan said.

I see what you mean. `virtual final` could unintentionally declare a new virtual function instead of overriding a base class's virtual function, if the base class's definition changed.

I'll post another patch to remove the virtual keywords from these final override patches.
When removing the `virtual` keyword from final virtual function declarations, I found four existing instances of virtual function declarations that are marked final but do not actually override a base class's virtual function. Three of these declarations appear to be leftover copy/paste code that I think can safely be made non-virtual:

* AccessibleNode::GetParentObject is a non-overriding final virtual function. GetParentObject is a common virtual function in many DOM classes, but AccessibleNode does not derive from any base classes the define GetParentObject or have any derived classes of its own:

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

* WebCryptoTask::CalculateResult and CallCallback are non-overriding final virtual functions that mirror virtual function names in the CryptoTask class, even though WebCryptoTask does not actually derive from CryptoTask:

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

* The fourth declaration, NOT_INHERITED_CANT_OVERRIDE, is a debug macro that uses a non-overriding final virtual function BaseCycleCollectable for some compile-time checks of CC-able classes. IIUC, I think this declaration needs to remain virtual/final because it is purposely wants to prevent derived classes from re-declaring NOT_INHERITED_CANT_OVERRIDE.

https://searchfox.org/mozilla-central/rev/a5abf843f8fac8530aa5de6fb40e16547bb4a47a/xpcom/base/nsCycleCollectionParticipant.h#619-629
Correction: There is also nsWindowBase::GetWindowHandle, which does not override any base class's virtual functions:

https://searchfox.org/mozilla-central/source/widget/windows/WinMouseScrollHandler.h#191

The only other function called GetWindowHandle is MouseScrollHandler::EventInfo::GetWindowHandle, which is not related to the nsWindowBase class:

https://searchfox.org/mozilla-central/rev/7ed2a637dc305e90fed6ffb041c6b8fa162b66bd/widget/windows/WinMouseScrollHandler.h#191
Masatoshi, I wrote this `mach lint` regex check that warns about:

  virtual void Example() final
  void Example() final override
  void Example() override final

Do you think this lint check is worthwhile? After the patches in this bug are applied, there are no warnings in Mozilla code.

Some caveats:

* It doesn't warn about `virtual void Example() override` at this time because there are 8000+ warnings.

* It doesn't warn about multi-line function declarations because the regex can't match across more than one line.

* Ehsan enabled clang-tidy's misc-use-override check in bug 1158776, but it was removed in bug 1420366 because it was too noisy (because it didn't understand how the NS_IMETHOD macro affects virtual functions).
Attachment #8949963 - Flags: feedback?(VYV03354)
Attachment #8949963 - Attachment mime type: text/x-vhdl → text/plain
Comment on attachment 8949963 [details]
virtual-final-override.yml

Makes sense.

(In reply to Chris Peterson [:cpeterson] from comment #17)
> * It doesn't warn about `virtual void Example() override` at this time
> because there are 8000+ warnings.

I think it is OK because it is not as bad as `virtual ... final`.
Attachment #8949963 - Flags: feedback?(VYV03354) → feedback+
Comment on attachment 8949959 [details]
Bug 1436263 - Part 4: Remove unnecessary `virtual` and `final` from non-overriding final virtual function declarations.

https://reviewboard.mozilla.org/r/219248/#review225274

::: commit-message-5cd2e:1
(Diff revision 1)
> +Bug 1436263 - Part 4: Remove unnecessary `virtual` and `final` from non-overriding final virtual funciton declarations. r?froydnj

Nit: "final virtual function"

::: commit-message-5cd2e:3
(Diff revision 1)
> +Bug 1436263 - Part 4: Remove unnecessary `virtual` and `final` from non-overriding final virtual funciton declarations. r?froydnj
> +
> +AccessibleNode::GetParentObject is a non-overriding final virtual function. GetParentObject is a common virtual function in many DOM classes, but AccessibleNode does not derive from any base classes the define virtual GetParentObject or have any derived classes of its own.

Nit: "any base classes that define virtual GetParentObject"?

::: commit-message-5cd2e:5
(Diff revision 1)
> +Bug 1436263 - Part 4: Remove unnecessary `virtual` and `final` from non-overriding final virtual funciton declarations. r?froydnj
> +
> +AccessibleNode::GetParentObject is a non-overriding final virtual function. GetParentObject is a common virtual function in many DOM classes, but AccessibleNode does not derive from any base classes the define virtual GetParentObject or have any derived classes of its own.
> +
> +WebCryptoTask::CalculateResult and CallCallback are non-overriding final virtual functions that mirror virtual function names in the CryptoTask class, even though WebCryptoTask does not actually derive from CryptoTask.

Having a `WebCryptoTask` that doesn't derive from `CryptoTask` seems like a terrible name collision. :(
Attachment #8949959 - Flags: review?(nfroyd) → review+
Comment on attachment 8949958 [details]
Bug 1436263 - Part 3: Remove `virtual` from final virtual function declarations.

https://reviewboard.mozilla.org/r/219246/#review225284

Thanks.

::: dom/base/nsDOMCaretPosition.h:90
(Diff revision 1)
> -  virtual JSObject* WrapObject(JSContext *aCx, JS::Handle<JSObject*> aGivenProto)
> +  JSObject* WrapObject(JSContext *aCx, JS::Handle<JSObject*> aGivenProto)
>      final;

Are we able to re-wrap this line?
Attachment #8949958 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #19)
> > +WebCryptoTask::CalculateResult and CallCallback are non-overriding final virtual functions that mirror virtual function names in the CryptoTask class, even though WebCryptoTask does not actually derive from CryptoTask.
> 
> Having a `WebCryptoTask` that doesn't derive from `CryptoTask` seems like a
> terrible name collision. :(

FWIW, WebCryptoTask used to derive from CryptoTask, but was decoupled in bug 1001691 (and bug 842818 comment 58) so it could be used on worker threads. CryptoTasks can only be created on the main thread. A common abstract base class could be extracted, but these classes aren't used together so there doesn't seem much value in doing so.
Comment on attachment 8951169 [details]
Bug 1436263 - Part 5: Add a mach lint for virtual function declarations with multiple specifiers.

https://reviewboard.mozilla.org/r/220430/#review226564

rs=me.  Kind of unfortunate that we can't warn across multiple lines, but I guess that's life?
Attachment #8951169 - Flags: review?(nfroyd) → review+
I think a clang-based checkers would be probably more effective in catching these cases (but harder to develop).
Comment on attachment 8951169 [details]
Bug 1436263 - Part 5: Add a mach lint for virtual function declarations with multiple specifiers.

https://reviewboard.mozilla.org/r/220430/#review226604

I do agree with Sylvestre that this would be better off as a static-analysis check, but don't want to let perfect be the enemy of good, so looks alright to me! The r- is for the taskcluster issue.

::: commit-message-b350a:1
(Diff revision 1)
> +Bug 1436263 - Part 5: Add a mach lint for virtual function declarations with multiple specifiers. r?froydnj r?ahal

Please add a taskcluster task in this file:
https://searchfox.org/mozilla-central/source/taskcluster/ci/source-test/mozlint.yml

Without the task, bustage could slip in without being backed out, and then people would start seeing failures that aren't their fault.
Attachment #8951169 - Flags: review?(ahalberstadt) → review-
(In reply to Andrew Halberstadt [:ahal] from comment #29)
> Please add a taskcluster task in this file:
> https://searchfox.org/mozilla-central/source/taskcluster/ci/source-test/
> mozlint.yml
> 
> Without the task, bustage could slip in without being backed out, and then
> people would start seeing failures that aren't their fault.

Thanks. I've added the task and confirmed that it can run successfully [1] and can catch virtual/final regressions [2].

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=052e679efdadd5b84618c3cf5ceec8915d5b0085

[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=e958fd1b069aace825faf42ee2dab35c938023f4
Blocks: 1158776
Comment on attachment 8951169 [details]
Bug 1436263 - Part 5: Add a mach lint for virtual function declarations with multiple specifiers.

https://reviewboard.mozilla.org/r/220430/#review226848

Thanks.
Attachment #8951169 - Flags: review?(ahalberstadt) → review+
Pushed by cpeterson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d864fe04c3ea
Part 1: Replace `final override` virtual function specifiers with just `final`. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/6064b17b6be6
Part 2: Replace `override final` virtual function specifiers with just `final`. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7634d84216c
Part 3: Remove `virtual` from final virtual function declarations. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f706e7a3934
Part 4: Remove unnecessary `virtual` and `final` from non-overriding final virtual function declarations. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a00e515def3
Part 5: Add a mach lint for virtual function declarations with multiple specifiers. r=froydnj r=ahal
Depends on: 1513950
You need to log in before you can comment on or make changes to this bug.