1430669 - (Wsuggest-override) Enable gcc -Wsuggest-override warnings

Status: RESOLVED WONTFIX

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

Description

[Chris Peterson [:cpeterson]]

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

Comment 1

[Masatoshi Kimura [:emk]]

Do we update the coding style every time some random new tools prefer a different style?

Comment 2

[Chris Peterson [:cpeterson]]

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).

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: Part 1: Suppress -Wsuggest-override warnings from Breakpad header files

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

Comment 4

[Chris Peterson [:cpeterson]]

Attachment: Part 2: Suppress -Wsuggest-override warnings from gtest header files

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

Comment 5

[Chris Peterson [:cpeterson]]

Attachment: Part 3: Suppress -Wsuggest-override warnings from protobuf header files

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.

Comment 6

[Chris Peterson [:cpeterson]]

Attachment: Part 4: Enable gcc -Wsuggest-override warnings

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

Comment 7

[Mike Hommey [:glandium]]

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)

Comment 8

[Chris Peterson [:cpeterson]]

(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.