build: Enable -Wsuggest-override if available #16710

hebasto · 2019-08-24T14:32:12Z

From GCC docs:

-Wsuggest-override
Warn about overriding virtual functions that are not marked with the override keyword.

~~This PR is based on #16722 (the first commit).~~ See: #16722 (comment)

practicalswift · 2019-08-24T14:49:13Z

Concept ACK -- consistent use of override prevents bugs

@hebasto Is this change complete in the sense that it addresses all instances of missing override in the code?

Note that clang's -Winconsistent-missing-override only checks for inconsistent override usage within a class. gcc:s -Wsuggest-override checks for missing overrides in general. clang-tidy:s modernize-use-override is helpful too.

DrahtBot · 2019-08-24T14:53:00Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

build: avoid repetitions when enabling warnings in configure.ac #18857 (build: avoid repetitions when enabling warnings in configure.ac by vasild)
qt: Remove QFont warnings with QT_QPA_PLATFORM=minimal #17908 (qt: Remove QFont warnings with QT_QPA_PLATFORM=minimal by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

hebasto · 2019-08-24T18:07:15Z

@practicalswift

Note that clang's -Winconsistent-missing-override only checks for inconsistent override usage within a class.

I did know that. Thank you.

hebasto · 2019-08-24T18:36:11Z

@practicalswift

Is this change complete in the sense that it addresses all instances of missing override in the code?

Yes, it is (with the latest push).

practicalswift · 2019-08-24T21:06:59Z

ACK 6a8d284 -- diff looks correct

Thanks for making the change complete across the codebase.

I think we should consider building with -Werror=suggest-override by default. But that is perhaps out of the scope this PR :-)

promag · 2019-08-25T00:12:58Z

Concept ACK.

But that is perhaps out of the scope this

Why? Otherwise this may be necessary again right?

hebasto · 2019-08-25T04:48:04Z

@practicalswift @promag Thank you for your reviews.

I think we should consider building with -Werror=suggest-override by default.

How to skip warnings on leveldb subtree and qt/paymentrequest.pb.h?

practicalswift · 2019-08-25T09:53:10Z

But that is perhaps out of the scope this

Why? Otherwise this may be necessary again right?

True! If @hebasto wants to incorporate -Werror=suggest-override as part of this PR I'm first in line to give a warm ACK :-)

practicalswift · 2019-08-25T09:56:33Z

@hebasto

How to skip warnings on leveldb subtree and qt/paymentrequest.pb.h?

Could you make use of -isystem? See examples in @Empact's PRs #14920 and #15112.

hebasto · 2019-08-25T13:39:46Z

@practicalswift

Could you make use of -isystem?

@theuni #14920 (comment)

I'm really not a fan of disabling warnings for 3rd party headers...

I tend to agree with @theuni regarding isystem approach ~~, so I'm going to leave this PR as it is for now~~.

hebasto · 2019-08-26T13:15:12Z

@practicalswift @promag

Why? Otherwise this may be necessary again right?

True! If @hebasto wants to incorporate -Werror=suggest-override as part of this PR I'm first in line to give a warm ACK :-)

The latest push incorporates -Wsuggest-override as a compiler option.

PR description has been updated.

hebasto · 2019-08-27T17:42:21Z

Fixed MSVC build.

hebasto · 2019-08-28T23:11:02Z

Fixed protobuf stuff.

hebasto · 2019-08-29T11:51:39Z

@practicalswift Would you mind re-reviewing this PR?

hebasto · 2019-08-30T04:08:00Z

Rebased.

laanwj · 2019-09-30T11:52:25Z

Concept ACK.

Not convinced about --enable-wleveldb. Unless the warnings are purely silly, I think we should try to get these problems solved upstream not hide them.
(and if you really want to have a different warning level for built-in dependencies, maybe make it general and cover secp256k1 too?)

I think we should consider building with -Werror=suggest-override by default. But that is perhaps out of the scope this PR :-)

NACK on WError of any kind by default. Feel free to enable it locally as a developer, but it is frustrating for packagers and people just trying to build the code, because changes in gcc version, dependency libraries and such frequently change the set of warnings.

hebasto · 2019-10-01T18:06:10Z

Not convinced about --enable-wleveldb.

It seems there is a commit google/leveldb@28e6d23 to upstream related to -Wsuggest-override.

Unless the warnings are purely silly, I think we should try to get these problems solved upstream not hide them.

E.g., #15539.

laanwj · 2019-10-03T05:21:14Z

It seems there is a commit google/leveldb@28e6d23 to upstream related to -Wsuggest-override.

Nice, let's pull that in. It might be time to do a leveldb update from upstream anyhow for 0.20.

E.g., #15539.

Yes, would agree those are silly. I guess we could disable specific warnings for leveldb, if there's no change of them being solved, I just dislike the idea of a blanket disable.

hebasto · 2020-05-11T18:47:42Z

Updated a96adef -> d44a13b (pr16710.14 -> pr16710.15):

rebased
addressed @vasild's comment:

I would suggest to not enable the new flag on LevelDB sources and to broaden the scope of --enable-werror like this...

I'd refrain from -Werror=suggest-override while we have buggy compilers like GCC < 9.2 (see the top commit).

maflcko · 2020-05-11T19:27:17Z

I don't think d44a13b is maintainable. How would devs with compilers that are not buggy figure out where to add the pragmas?

I think the only thing that can be done is enable the warning for compilers that support it and skip it if the compiler does not support it fully and correctly.

vasild · 2020-05-11T19:28:26Z

ACK d44a13b

The current code contains just a few final methods so it is kind of ok to surround them with #pragma suppress if gcc < 9.2.

However, every addition of a new final method would either produce a warning with gcc < 9.2 or require to be also surrounded by the #pragma stuff. I wonder if it would be better to only conditionally enable -Wsuggest-override in configure.ac if the compiler is not gcc < 9.2?

hebasto · 2020-05-12T09:10:56Z

Updated d44a13b -> ea6849f (pr16710.15 -> pr16710.16, diff):

-Wsuggest-override and -Werror=suggest-override are enabled only for non-buggy GCC versions, i.e. 9.2+.

hebasto · 2020-05-12T09:13:10Z

@theuni

But it seems overkill to "turn the warnings off wholesale". Especially considering that "buggy" GCC just fires some false-positive warnings, which could be easily suppressed in well-documented manner.

If a warning flag is known to be buggy and it can be detected, we should turn it off. It's perfectly reasonable imo to say "If override warnings for this compiler are known to be broken, don't enable them".

Done in the latest push.

configure.ac

hebasto · 2020-05-12T10:08:14Z

Updated ea6849f -> e6211d8 (pr16710.16 -> pr16710.17, diff):

addressed @vasild's comments

vasild

ACK e6211d8

GCC 8.4.0:

checking for GCC version < 9.2... yes
configure: WARNING: GCC<9.2 reports a redundant -Wsuggest-override warning on a 'final' method, disabling -Wsuggest-override.

(and -Wsuggest-override -Werror=suggest-override are not in CXXFLAGS)

GCC 9.3.0:

checking for GCC version < 9.2... no
checking whether C++ compiler accepts -Werror=suggest-override... yes
checking whether C++ compiler accepts -Wsuggest-override... yes

Clang 11.0.0:

checking for GCC version < 9.2... no
checking whether C++ compiler accepts -Werror=suggest-override... no
checking whether C++ compiler accepts -Wsuggest-override... no

fanquake

How about something like this to replace the changes in configure.ac:

diff --git a/configure.ac b/configure.ac
index 7de6adb93..31e9c243f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -368,6 +368,11 @@ if test "x$enable_werror" = "xyes"; then
   AX_CHECK_COMPILE_FLAG([-Werror=return-type],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=return-type"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Werror=conditional-uninitialized],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=conditional-uninitialized"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Werror=sign-compare],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=sign-compare"],,[[$CXXFLAG_WERROR]])
+
+  dnl -Wsuggest-override is  broken with GCC before 9.2
+  dnl https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78010
+  AX_CHECK_COMPILE_FLAG([-Wsuggest-override],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wsuggest-override"],,[[$CXXFLAG_WERROR]],
+                       [AC_LANG_PROGRAM([[struct A { virtual void f(); };struct B : A { void f() final; };]])])
 fi
 
 if test "x$CXXFLAGS_overridden" = "xno"; then
@@ -385,6 +390,8 @@ if test "x$CXXFLAGS_overridden" = "xno"; then
   AX_CHECK_COMPILE_FLAG([-Wdate-time],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wdate-time"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Wconditional-uninitialized],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wconditional-uninitialized"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Wsign-compare],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wsign-compare"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Wsuggest-override],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wsuggest-override"],,[[$CXXFLAG_WERROR]],
+                       [AC_LANG_PROGRAM([[struct A { virtual void f(); };struct B : A { void f() final; };]])])
 
   dnl Some compilers (gcc) ignore unknown -Wno-* options, but warn about all
   dnl unknown options if any other warning is produced. Test the -Wfoo case, and

This way the check stays self-contained, no need to test version macros, no additional configure output and we can just use the source from the bug report.

configure.ac

hebasto · 2020-05-12T15:09:48Z

@fanquake Thank you for your review. TIL :)

Updated e6211d8 -> 839add1 (pr16710.17 -> pr16710.18, diff).

addressed @fanquake's comment

practicalswift · 2020-05-12T15:11:02Z

ACK 839add1 assuming Travis is happy: patch looks correct

vasild · 2020-05-12T15:31:13Z

ACK 839add1

Even better! I confirm it works as intended with GCC 8.4, 9.3 and Clang 11.0.

fanquake

ACK 839add1

839add1 build: Enable -Wsuggest-override (Hennadii Stepanov) de5e91c refactor: Add BerkeleyDatabaseVersion() function (Hennadii Stepanov) Pull request description: From GCC [docs](https://gcc.gnu.org/onlinedocs/gcc-9.2.0/gcc/Warning-Options.html): > `-Wsuggest-override` > Warn about overriding virtual functions that are not marked with the override keyword. ~This PR is based on bitcoin#16722 (the first commit).~ See: bitcoin#16722 (comment) ACKs for top commit: fanquake: ACK 839add1 vasild: ACK 839add1 practicalswift: ACK 839add1 assuming Travis is happy: patch looks correct Tree-SHA512: 1e8cc085da30d41536deff9b181962c1882314ab252c2ad958294087ae1e5a0dfa4886bdbe36f21cf6ae71df776a8420f349f007d4b5b49fd79ba98ce308965a

Summary: refactor: Use override for non-final overriders 1551cea2d52cac403ff506a7cc955d8de8fd6f3e refactor: Remove override for final overriders d044e0ec7d37bbcdf10bbdb903b9119741c7297d > Two commits are split out from [[bitcoin/bitcoin#16710 | PR16710]] to make reviewing easier. > > From C++ FAQ: > > C.128: Virtual functions should specify exactly one of virtual, override, or final > Reason Readability. Detection of mistakes. Writing explicit virtual, override, or final is self-documenting and enables the compiler to catch mismatch of types and/or names between base and derived classes. However, writing more than one of these three is both redundant and a potential source of errors. This is a backport of Core [[bitcoin/bitcoin#18914 | PR18914]] Most of the work was done already in D767 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9082

merge bitcoin#18914, bitcoin#13306, bitcoin#16424, bitcoin#13899, bitcoin#17486, bitcoin#17880, bitcoin#18145, bitcoin#18843, bitcoin#16710: split warnings out of CXXFLAGS, add more flags

Summary: This is a backport of [[bitcoin/bitcoin#16710 | core#16710]] [1/2] bitcoin/bitcoin@de5e91c This is backported out of order. In the original PR, it should have touched db.{h|c}, but the code was moved to bdb.{h|c} in D8615. The original PR includes only `db.h` in `rpcconsole.cpp`, and `bdb.h` should have been added additionaly in D8615 (both includes will be needed for subsequent backports (e.g. core#20202). Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10209

fanquake added GUI Refactoring labels Aug 24, 2019

hebasto force-pushed the 20190824-consistent-override branch from d6f5d89 to f0e0a46 Compare August 24, 2019 15:40

hebasto force-pushed the 20190824-consistent-override branch from f0e0a46 to 6a8d284 Compare August 24, 2019 18:30

hebasto mentioned this pull request Aug 25, 2019

build: Disable warnings for leveldb subtree by default #16722

Closed

hebasto force-pushed the 20190824-consistent-override branch from 6a8d284 to cf6bcb5 Compare August 26, 2019 13:04

hebasto changed the title ~~refactor: Use 'override' specifier for all overriding functions~~ build: Enable -Wsuggest-override if available Aug 26, 2019

hebasto force-pushed the 20190824-consistent-override branch from cf6bcb5 to 309bb12 Compare August 27, 2019 17:40

hebasto force-pushed the 20190824-consistent-override branch from 309bb12 to 404c965 Compare August 28, 2019 23:08

DrahtBot added the Needs rebase label Aug 29, 2019

hebasto force-pushed the 20190824-consistent-override branch from 404c965 to 695ad19 Compare August 30, 2019 04:07

DrahtBot removed the Needs rebase label Aug 30, 2019

DrahtBot removed the Needs rebase label May 11, 2020

hebasto force-pushed the 20190824-consistent-override branch from d44a13b to ea6849f Compare May 12, 2020 08:44

vasild reviewed May 12, 2020

View reviewed changes

configure.ac Outdated Show resolved Hide resolved

configure.ac Outdated Show resolved Hide resolved

hebasto force-pushed the 20190824-consistent-override branch from ea6849f to e6211d8 Compare May 12, 2020 10:06

vasild approved these changes May 12, 2020

View reviewed changes

fanquake requested changes May 12, 2020

View reviewed changes

configure.ac Outdated Show resolved Hide resolved

build: Enable -Wsuggest-override

839add1

hebasto force-pushed the 20190824-consistent-override branch from e6211d8 to 839add1 Compare May 12, 2020 15:05

fanquake approved these changes May 13, 2020

View reviewed changes

fanquake merged commit 219c55d into bitcoin:master May 13, 2020

hebasto deleted the 20190824-consistent-override branch May 13, 2020 12:51

kwvg added a commit to kwvg/dash that referenced this pull request Jun 16, 2021

bitcoin#16710: Enable -Wsuggest-override if available

6dd90ea

kwvg added a commit to kwvg/dash that referenced this pull request Jun 17, 2021

bitcoin#16710: Enable -Wsuggest-override if available

93b76fb

kwvg added a commit to kwvg/dash that referenced this pull request Jun 17, 2021

bitcoin#16710: Enable -Wsuggest-override if available

31e192f

kwvg added a commit to kwvg/dash that referenced this pull request Jun 17, 2021

bitcoin#16710: Enable -Wsuggest-override if available

4802b41

kwvg added a commit to kwvg/dash that referenced this pull request Jun 17, 2021

bitcoin#16710: Enable -Wsuggest-override if available

b07abef

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

build: Enable -Wsuggest-override if available #16710

build: Enable -Wsuggest-override if available #16710

Uh oh!

Conversation

hebasto commented Aug 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Aug 24, 2019

Uh oh!

DrahtBot commented Aug 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

hebasto commented Aug 24, 2019

Uh oh!

hebasto commented Aug 24, 2019

Uh oh!

practicalswift commented Aug 24, 2019

Uh oh!

promag commented Aug 25, 2019

Uh oh!

hebasto commented Aug 25, 2019

Uh oh!

practicalswift commented Aug 25, 2019

Uh oh!

practicalswift commented Aug 25, 2019

Uh oh!

hebasto commented Aug 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Aug 26, 2019

Uh oh!

hebasto commented Aug 27, 2019

Uh oh!

hebasto commented Aug 28, 2019

Uh oh!

hebasto commented Aug 29, 2019

Uh oh!

hebasto commented Aug 30, 2019

Uh oh!

laanwj commented Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Oct 1, 2019

Uh oh!

laanwj commented Oct 3, 2019

Uh oh!

hebasto commented May 11, 2020

Uh oh!

maflcko commented May 11, 2020

Uh oh!

vasild commented May 11, 2020

Uh oh!

hebasto commented May 12, 2020

Uh oh!

hebasto commented May 12, 2020

Uh oh!

Uh oh!

Uh oh!

hebasto commented May 12, 2020

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto commented May 12, 2020

Uh oh!

practicalswift commented May 12, 2020

Uh oh!

vasild commented May 12, 2020

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto commented Aug 24, 2019 •

edited

Loading

DrahtBot commented Aug 24, 2019 •

edited

Loading

hebasto commented Aug 25, 2019 •

edited

Loading

laanwj commented Sep 30, 2019 •

edited

Loading