Closed
Bug 1428535
Opened 6 years ago
Closed 6 years ago
Add missing override specifiers and remove redundant virtual specifiers on overridden virtual functions
Categories
(Core :: General, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla59
People
(Reporter: cpeterson, Assigned: cpeterson)
References
Details
Attachments
(3 files)
1. Add missing override specifiers to overridden virtual functions identified by gcc 5.0's -Wsuggest-override warnings. 2. Remove redundant virtual specifiers for (single-line) function declarations that specify both virtual and override. Cleaning up redundant virtual specifiers from multi-line function declarations will take some more work and generate large patches to fix indentation. I have follow-up patches to enable gcc's -Wsuggest-override warnings, but I'd like to land these patches to add missing overrides first. These patches bitrot quickly and I want to make sure you are happy with this approach before I bother glandium to review the warning patches. Mozilla's Coding Style Guide says: "Method declarations must use at most one of the following keywords: virtual, override, or final." This should be updated to replace `final` with `final override` because gcc's -Wsuggest-override warnings do not assume `final` implies `override`. A member function could be virtual and final without being override, but that is not a useful combination. Something like: "Method declarations must use at most one of the following specifiers: `virtual`, `override`, or `final override`." https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•6 years ago
|
||
mozreview-review |
Comment on attachment 8940411 [details] Bug 1428535 - Part 1: Add missing override specifiers to overridden virtual functions. https://reviewboard.mozilla.org/r/210688/#review216706 I skimmed through the first page, and all of this looks reasonable. r=me + r=me on whatever small fixups might be necessary to land these patches.
Attachment #8940411 -
Flags: review?(nfroyd) → review+
Comment 5•6 years ago
|
||
mozreview-review |
Comment on attachment 8940412 [details] Bug 1428535 - Part 2: Remove redundant virtual specifiers from overridden virtual functions. https://reviewboard.mozilla.org/r/210690/#review216708 r=me
Attachment #8940412 -
Flags: review?(nfroyd) → review+
Comment 6•6 years ago
|
||
mozreview-review |
Comment on attachment 8940413 [details] Bug 1428535 - Part 3: Emit override specifiers in generated ipc/ipdl code. https://reviewboard.mozilla.org/r/210692/#review216712 I think the Right Thing here is to make construction of `MethodDecl` not terrible, so we can specify `override=1` at construction time and have that imply `virtual`, rather than having to specify both. And `pure=1` would work similarly. Maybe we could even consolidate multiple keyword args into an enum as you suggest. But changing that seems like a little much for this patch. And this is `ipdl.py`, nobody is going to get too upset about verbosity. Please file a followup to clean that up, though.
Attachment #8940413 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 7•6 years ago
|
||
I'm landing the ipdl patch now. I'll land the large override/virtual patches on the weekend when they are less likely to cause merge conflicts for the sheriffs. I filed bug 1428984 for the ipdl follow-up from comment 6.
Keywords: leave-open
Pushed by cpeterson@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/79dbd796c874 Part 3: Emit override specifiers in generated ipc/ipdl code. r=froydnj
Comment 9•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/79dbd796c874
Comment 10•6 years ago
|
||
Pushed by cpeterson@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3d28ca7c05d8 Add missing override specifiers to overridden virtual functions. r=froydnj
Assignee | ||
Comment 11•6 years ago
|
||
I decided to not land patch #2 to remove redundant virtual specifiers. The patch is large and likely to cause merge conflicts for sheriffs or developers, while its benefit is just a style cleanup.
Keywords: leave-open
Comment 12•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3d28ca7c05d8
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Assignee | ||
Updated•6 years ago
|
Blocks: Wsuggest-override
Comment 13•6 years ago
|
||
Pushed by cpeterson@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7d9ba3d64962 Revert some unnecessary -Wsuggest-override warning suppressions.
Assignee | ||
Comment 14•6 years ago
|
||
It was decided in bug 1430669 that we won't enable gcc's -Wsuggest-override warnings at this time, so these ugly pragmas from bug 1428535 won't be necessary.
Comment 15•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7d9ba3d64962
Comment 16•6 years ago
|
||
> "Method declarations must use at most one of the following specifiers: `virtual`, `override`, or `final override`."
I feel very sad about this.
IIRC, a single "final" indicates "override" (although a combination of "virtual" and "final" doesn't).
We should ask GCC to fix that instead of appending override to places where it's not necessary. And we should just suppress that warning in the mean time.
Because this new rule is very easily broken by people developing with non-GCC builds, and they can only realize that when pushing to try. This is not very productive.
Comment 17•6 years ago
|
||
Sounds like the warning is still suppressed, but there are lots of "override" added to methods which have "final". Shouldn't we revert that change as well?
Flags: needinfo?(cpeterson)
Assignee | ||
Comment 18•6 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #17) > Sounds like the warning is still suppressed, but there are lots of > "override" added to methods which have "final". Shouldn't we revert that > change as well? I landed the patches to add missing overrides, but because of the reasons you mention, I did not enable gcc's -Wsuggest-override warnings and have no plans to. (There is more discussion in bug 1430669.) So we don't need to update Mozilla's C++ coding style guide to specify `final override`.
Flags: needinfo?(cpeterson)
Comment 19•6 years ago
|
||
(In reply to Chris Peterson [:cpeterson] from comment #18) > (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #17) > > Sounds like the warning is still suppressed, but there are lots of > > "override" added to methods which have "final". Shouldn't we revert that > > change as well? > > I landed the patches to add missing overrides, but because of the reasons > you mention, I did not enable gcc's -Wsuggest-override warnings and have no > plans to. (There is more discussion in bug 1430669.) So we don't need to > update Mozilla's C++ coding style guide to specify `final override`. So, shouldn't we turn those `final override` by your patch back to `final` given that we are not going to change our coding style?
Assignee | ||
Comment 20•6 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #19) > So, shouldn't we turn those `final override` by your patch back to `final` > given that we are not going to change our coding style? Sure. I can write a new patch to remove all `final override` (and `override final`) specifiers, both those add by this bug added and those that already existed. https://searchfox.org/mozilla-central/search?q=final+override (723 instances) https://searchfox.org/mozilla-central/search?q=override+final (136 instances)
Comment 21•6 years ago
|
||
Part 1 added "virtual" modifiers to some methods in js/src/irregexp which don't look correct to me. Specifically these three methods are now annotated as "virtual" even though they're never overridden in subclasses. BackReferenceNode::end_register() ChoiceNode::EatsAtLeastHelper(...) LoopChoiceNode::AddContinueAlternative(...) I haven't checked if there are more cases with the same issue.
Comment 22•6 years ago
|
||
cpeterson, do you know what happened RE comment 21? The commit message for "part 1" suggests that it only intended to add "override" keywords, not that it was adding "virtual" keywords. Not sure if this was an automated tool vs. human error vs. something that's subtly necessary for non-obvious reasons. For convenience, here's the searchfox view of commit's changes that anba was hinting at, with functions being promoted to virtual: * end_register: https://searchfox.org/mozilla-central/diff/37efe4d0e6d74152023e5e142f670e6a2cec478c/js/src/irregexp/RegExpEngine.h#892 * EatsAtLeastHelper: https://searchfox.org/mozilla-central/diff/37efe4d0e6d74152023e5e142f670e6a2cec478c/js/src/irregexp/RegExpEngine.h#1033 * AddContinueAlternative: https://searchfox.org/mozilla-central/diff/37efe4d0e6d74152023e5e142f670e6a2cec478c/js/src/irregexp/RegExpEngine.h#1120
Flags: needinfo?(cpeterson)
Comment 23•6 years ago
|
||
FWIW I'll revert these changes to js/src/irregexp as part of bug 1367105.
Assignee | ||
Comment 24•6 years ago
|
||
(In reply to André Bargull [:anba] from comment #21) > Part 1 added "virtual" modifiers to some methods in js/src/irregexp which > don't look correct to me. Specifically these three methods are now annotated > as "virtual" even though they're never overridden in subclasses. > > BackReferenceNode::end_register() > ChoiceNode::EatsAtLeastHelper(...) > LoopChoiceNode::AddContinueAlternative(...) I'm not sure what happened. Those changes are incorrect. Thanks for finding them! I just grepped through the diff from https://hg.mozilla.org/integration/mozilla-inbound/rev/3d28ca7c05d8 for modified lines with `virtual` but not `override`. I only found the same functions that André already found: + virtual int EatsAtLeastHelper(int still_to_find, + virtual int end_register() { return end_reg_; } + virtual void AddContinueAlternative(GuardedAlternative alt);
Flags: needinfo?(cpeterson)
You need to log in
before you can comment on or make changes to this bug.
Description
•