Closed Bug 1428535 Opened 2 years ago Closed 2 years ago

Add missing override specifiers and remove redundant virtual specifiers on overridden virtual functions

Categories

(Core :: General, defect, P3)

Unspecified
All
defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- wontfix
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

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 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 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 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+
Depends on: 1428984
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
Pushed by cpeterson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d28ca7c05d8
Add missing override specifiers to overridden virtual functions. r=froydnj
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
https://hg.mozilla.org/mozilla-central/rev/3d28ca7c05d8
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Pushed by cpeterson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d9ba3d64962
Revert some unnecessary -Wsuggest-override warning suppressions.
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.
> "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.
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)
(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)
(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?
(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)
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.
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)
FWIW I'll revert these changes to js/src/irregexp as part of bug 1367105.
(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)
Depends on: 1436263
Blocks: 1158776
You need to log in before you can comment on or make changes to this bug.