Closed
Bug 1436263
Opened 6 years ago
Closed 6 years ago
Replace `final override` virtual function specifiers with just `final`
Categories
(Core :: General, defect, P3)
Core
General
Tracking
()
RESOLVED
FIXED
mozilla60
People
(Reporter: cpeterson, Assigned: cpeterson)
References
Details
Attachments
(6 files)
59 bytes,
text/x-review-board-request
|
froydnj
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
froydnj
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
froydnj
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
froydnj
:
review+
|
Details |
747 bytes,
text/plain
|
emk
:
feedback+
|
Details |
59 bytes,
text/x-review-board-request
|
ahal
:
review+
froydnj
:
review+
|
Details |
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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•6 years ago
|
||
And we should add static analysis rule. It is particularly important to prevent `virtual foo() final;`.
Comment 4•6 years ago
|
||
mozreview-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/#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 5•6 years ago
|
||
mozreview-review |
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 6•6 years ago
|
||
mozreview-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+
Assignee | ||
Comment 7•6 years ago
|
||
(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.
Comment 8•6 years ago
|
||
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.
Comment 9•6 years ago
|
||
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.
Assignee | ||
Comment 10•6 years ago
|
||
(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.
Assignee | ||
Comment 11•6 years ago
|
||
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
Assignee | ||
Comment 12•6 years ago
|
||
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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•6 years ago
|
||
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)
Updated•6 years ago
|
Attachment #8949963 -
Attachment mime type: text/x-vhdl → text/plain
Comment 18•6 years ago
|
||
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 19•6 years ago
|
||
mozreview-review |
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 20•6 years ago
|
||
mozreview-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+
Assignee | ||
Comment 21•6 years ago
|
||
(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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 27•6 years ago
|
||
mozreview-review |
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+
Comment 28•6 years ago
|
||
I think a clang-based checkers would be probably more effective in catching these cases (but harder to develop).
Comment 29•6 years ago
|
||
mozreview-review |
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-
Assignee | ||
Comment 30•6 years ago
|
||
(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
Comment hidden (mozreview-request) |
Comment 32•6 years ago
|
||
mozreview-review |
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+
Comment 33•6 years ago
|
||
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
Comment 34•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d864fe04c3ea https://hg.mozilla.org/mozilla-central/rev/6064b17b6be6 https://hg.mozilla.org/mozilla-central/rev/b7634d84216c https://hg.mozilla.org/mozilla-central/rev/8f706e7a3934 https://hg.mozilla.org/mozilla-central/rev/0a00e515def3
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in
before you can comment on or make changes to this bug.
Description
•