Closed Bug 1430669 (Wsuggest-override) Opened 2 years ago Closed 2 years ago

Enable gcc -Wsuggest-override warnings

Categories

(Firefox Build System :: General, enhancement, P3)

enhancement

Tracking

(firefox-esr52 wontfix, firefox57 wontfix, firefox58 wontfix, firefox59 affected)

RESOLVED WONTFIX
Tracking Status
firefox-esr52 --- wontfix
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- affected

People

(Reporter: cpeterson, Assigned: cpeterson)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

Warn when a virtual function is overridden but not marked `override`. Mozilla's Coding Style guide [1] says:

> Method declarations must use at most one of the following keywords: virtual,
> override, or final.  Use virtual to declare virtual methods which do not
> override a base class method with the same signature.  Use override to
> declare virtual methods which do override a base class method with the same
> signature, but can be further overridden in derived classes.  Use final to
> declare virtual methods which do override a base class method with the same
> signature, but cannot be further overridden in the derived classes.

gcc's -Wsuggest-override warning is similar to clang's -Winconsistent-missing-override warning, which is enabled by default. We need to suppress gcc's -Wsuggest-override warnings for some third-party code and Mozilla code that #includes affected third-party headers. Bug 1428535 added missing override specifiers to Mozilla code.

The -Wsuggest-override flag was first added in gcc 5.0.0. sixgill hazard analysis uses an older gcc version and doesn't understand "-Wno-suggest-override", so we must check gcc version in moz.build before suppressing the warning.

The coding style guide will need to be updated with s/final/final override/ because gcc's -Wsuggest-override does not assume final implies override, while clang's -Winconsistent-missing-override does. A virtual member function can be declared final but not override, though I'm not sure when this would be useful.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
Do we update the coding style every time some random new tools prefer a different style?
I can file a gcc bug. Assuming it is eventually fixed, we can adapt our coding style in the meantime, wait to land these -Wsuggest-override patches, or choose to never land these -Wsuggest-override patches.

There is another bug in Bugzilla somewhere (related to modernize-use-override bug 1420366?) suggesting we write our own clang-tidy check for missing overrides that can use a coding style of Mozilla's choosing and work on all platforms supporting clang (not just gcc).
The -Wsuggest-override flag was first added in gcc 5.0.0. sixgill hazard analysis uses an older gcc version and doesn't understand "-Wno-suggest-override", so we must check gcc version in moz.build before suppressing the warning.

MozReview-Commit-ID: 2U1ciWLjcjz
Attachment #8942805 - Flags: review?(mh+mozilla)
The -Wsuggest-override flag was first added in gcc 5.0.0. sixgill hazard analysis uses an older gcc version and doesn't understand "-Wno-suggest-override", so we must check gcc version in moz.build before suppressing the warning.

MozReview-Commit-ID: APEkffXZqcG
Attachment #8942807 - Flags: review?(mh+mozilla)
The -Wsuggest-override flag was first added in gcc 5.0.0. sixgill hazard analysis uses an older gcc version and doesn't understand "-Wno-suggest-override", so we must check gcc version in moz.build before suppressing the warning.

Also move -Wno-maybe-uninitialized behind a gcc compiler check because it is a gcc-specific flag.
Attachment #8942808 - Flags: review?(mh+mozilla)
Warn when a virtual function is overridden but not marked `override`. gcc's -Wsuggest-override warning is similar to clang's -Winconsistent-missing-override warning, which is enabled by default. We need to suppress gcc's -Wsuggest-override warnings for some third-party code and Mozilla code that #includes affected third-party headers.

The -Wsuggest-override flag was first added in gcc 5.0.0. sixgill hazard analysis uses an older gcc version and doesn't understand "-Wno-suggest-override", so we must check gcc version in moz.build before suppressing the warning.

MozReview-Commit-ID: HiEahIGPTZk
Attachment #8942809 - Flags: review?(mh+mozilla)
That's a lot of exceptions to have... I'm not sure we really want that.

I'll also note that the string comparison is only working by chance, and that's not a good precedent to put there.

If we really go forward with this, it would be better to have some variable set from configure that you can then use in moz.build like: CXXFLAGS += CONFIG['NO_SUGGEST_OVERRIDE'] (so that variable should be a list, empty when -Wsuggest-override is not supported)
Attachment #8942805 - Flags: review?(mh+mozilla)
Attachment #8942807 - Flags: review?(mh+mozilla)
Attachment #8942808 - Flags: review?(mh+mozilla)
Attachment #8942809 - Flags: review?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #7)
> That's a lot of exceptions to have... I'm not sure we really want that.

OK. I see what you mean. All those exceptions might be a pain to keep up to date when people add or update third-party code.

We already get some of the benefit of gcc's -Wsuggest-override warnings with clang's -Winconsistent-missing-override now that all the currently missing overrides were added in bug 1428535. Any new virtual functions that are added to an existing class with other virtual functions will be checked by clang.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.