build: Enable -Wsuggest-override if available

hebasto commented at 2:32 pm on August 24, 2019: member

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)

fanquake added the label GUI on Aug 24, 2019

fanquake added the label Refactoring on Aug 24, 2019

practicalswift commented at 2:49 pm on August 24, 2019: contributor

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 commented at 2:53 pm on August 24, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18857 (build: avoid repetitions when enabling warnings in configure.ac by vasild)
#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 force-pushed on Aug 24, 2019

hebasto commented at 6:07 pm on August 24, 2019: member

@practicalswift

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

I did know that. Thank you.

hebasto force-pushed on Aug 24, 2019

hebasto commented at 6:36 pm on August 24, 2019: member

@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 commented at 9:06 pm on August 24, 2019: contributor

ACK 6a8d284bf7595bd200cb27973a262c929cb72e14 – 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 commented at 0:12 am on August 25, 2019: member

Concept ACK.

But that is perhaps out of the scope this

Why? Otherwise this may be necessary again right?

hebasto commented at 4:48 am on August 25, 2019: member

@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 commented at 9:53 am on August 25, 2019: contributor

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 commented at 9:56 am on August 25, 2019: contributor

@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 commented at 1:39 pm on August 25, 2019: member

@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 force-pushed on Aug 26, 2019

hebasto renamed this:
~~refactor: Use 'override' specifier for all overriding functions~~
build: Enable -Wsuggest-override if available
on Aug 26, 2019

hebasto commented at 1:15 pm on August 26, 2019: member

@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 force-pushed on Aug 27, 2019

hebasto commented at 5:42 pm on August 27, 2019: member

Fixed MSVC build.

hebasto force-pushed on Aug 28, 2019

hebasto commented at 11:11 pm on August 28, 2019: member

Fixed protobuf stuff.

hebasto commented at 11:51 am on August 29, 2019: member

@practicalswift Would you mind re-reviewing this PR?

DrahtBot added the label Needs rebase on Aug 29, 2019

hebasto force-pushed on Aug 30, 2019

hebasto commented at 4:08 am on August 30, 2019: member

Rebased.

DrahtBot removed the label Needs rebase on Aug 30, 2019

laanwj commented at 11:52 am on September 30, 2019: member

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 commented at 6:06 pm on October 1, 2019: member

Not convinced about --enable-wleveldb.

It seems there is a commit https://github.com/google/leveldb/commit/28e6d238be73e743c963fc0a26395b783a7565e2 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 commented at 5:21 am on October 3, 2019: member

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.

DrahtBot added the label Needs rebase on Oct 26, 2019

hebasto force-pushed on Nov 2, 2019

hebasto commented at 1:40 pm on November 2, 2019: member

Rebased.

DrahtBot removed the label Needs rebase on Nov 2, 2019

practicalswift commented at 2:38 pm on November 2, 2019: contributor

ACK 46047bc4e2bb9063e35fb56b17236eead4052ea8 – diff looks correct

Thanks for keeping this PR rebased!

DrahtBot added the label Needs rebase on Nov 15, 2019

hebasto force-pushed on Feb 10, 2020

hebasto commented at 7:15 pm on February 10, 2020: member

Rebased and dropped a commit from #16722; see: #16722 (comment).

There are 3 warnings from the src/leveldb subtree only:

 0$ gcc --version | grep gcc
 1gcc (Ubuntu 7.4.0-1ubuntu1~18.04.1) 7.4.0
 2$ ./autogen.sh
 3$ ./configure
 4$ make -j 4 > /dev/null
 5/usr/bin/ar: `u' modifier ignored since `D' is the default (see `U')
 6ar: `u' modifier ignored since `D' is the default (see `U')
 7leveldb/db/c.cc: In function ‘leveldb_filterpolicy_t* leveldb_filterpolicy_create_bloom(int)’:
 8leveldb/db/c.cc:474:17: warning: ‘virtual const char* leveldb_filterpolicy_create_bloom(int)::Wrapper::Name() const’ can be marked override [-Wsuggest-override]
 9     const char* Name() const { return rep_->Name(); }
10                 ^~~~
11leveldb/db/c.cc:475:10: warning: ‘virtual void leveldb_filterpolicy_create_bloom(int)::Wrapper::CreateFilter(const leveldb::Slice*, int, std::__cxx11::string*) const’ can be marked override [-Wsuggest-override]
12     void CreateFilter(const Slice* keys, int n, std::string* dst) const {
13          ^~~~~~~~~~~~
14leveldb/db/c.cc:478:10: warning: ‘virtual bool leveldb_filterpolicy_create_bloom(int)::Wrapper::KeyMayMatch(const leveldb::Slice&, const leveldb::Slice&) const’ can be marked override [-Wsuggest-override]
15     bool KeyMayMatch(const Slice& key, const Slice& filter) const {
16          ^~~~~~~~~~~
17/usr/bin/ar: `u' modifier ignored since `D' is the default (see `U')

DrahtBot removed the label Needs rebase on Feb 10, 2020

DrahtBot added the label Needs rebase on Feb 21, 2020

hebasto force-pushed on Feb 25, 2020

hebasto commented at 7:19 pm on February 25, 2020: member

Rebased after #18034 has been merged.

practicalswift commented at 7:26 pm on February 25, 2020: contributor

ACK ec530398dbb3138d26c4783f880e4efe439fc450 – looks good: let’s get this merged :)

DrahtBot removed the label Needs rebase on Feb 25, 2020

Empact commented at 11:08 pm on February 27, 2020: member

Code Review ACK https://github.com/bitcoin/bitcoin/pull/16710/commits/ec530398dbb3138d26c4783f880e4efe439fc450 - also confirmed moves out of Q_SLOTS were appropriate.

in src/script/descriptor.cpp:517 in ec530398db outdated

513@@ -514,7 +514,7 @@ class AddressDescriptor final : public DescriptorImpl
514     std::vector<CScript> MakeScripts(const std::vector<CPubKey>&, const CScript*, FlatSigningProvider&) const override { return Vector(GetScriptForDestination(m_destination)); }
515 public:
516     AddressDescriptor(CTxDestination destination) : DescriptorImpl({}, {}, "addr"), m_destination(std::move(destination)) {}
517-    bool IsSolvable() const final { return false; }
518+    bool IsSolvable() const override final { return false; }

MarcoFalke commented at 8:50 pm on March 1, 2020:

This is final, but not virtual, so override is already implied. I am not sure why the compiler warns here?!

fanquake commented at 3:19 am on March 2, 2020:

GCC (Debian 8.3.0-6) will warn about this:

0  CXX      script/libbitcoin_common_a-descriptor.o
1script/descriptor.cpp:517:10: warning: 'virtual bool {anonymous}::AddressDescriptor::IsSolvable() const' can be marked override [-Wsuggest-override]
2     bool IsSolvable() const final { return false; }
3          ^~~~~~~~~~

However clang-tidy with modernize-use-override will tell you that it, along with a bunch of the other overrides added here are redundant:

 0src/script/descriptor.cpp:378:10: warning: 'override' is redundant since the function is already declared 'final' [modernize-use-override]
 1    bool IsRange() const override final
 2         ^               ~~~~~~~~~
 3src/script/descriptor.cpp:414:17: warning: 'override' is redundant since the function is already declared 'final' [modernize-use-override]
 4    std::string ToString() const override final
 5                ^                ~~~~~~~~~
 6src/script/descriptor.cpp:421:10: warning: 'override' is redundant since the function is already declared 'final' [modernize-use-override]
 7    bool ToPrivateString(const SigningProvider& arg, std::string& out) const override final
 8         ^                                                                   ~~~~~~~~~
 9src/script/descriptor.cpp:480:10: warning: 'override' is redundant since the function is already declared 'final' [modernize-use-override]
10    bool Expand(int pos, const SigningProvider& provider, std::vector<CScript>& output_scripts, FlatSigningProvider& out, std::vector<unsigned char>* cache = nullptr) const override final
11         ^                                                                                                                                                                   ~~~~~~~~~
12src/script/descriptor.cpp:485:10: warning: 'override' is redundant since the function is already declared 'final' [modernize-use-override]
13    bool ExpandFromCache(int pos, const std::vector<unsigned char>& cache, std::vector<CScript>& output_scripts, FlatSigningProvider& out) const override final
14         ^                                                                                                                                       ~~~~~~~~~
15src/script/descriptor.cpp:491:10: warning: 'override' is redundant since the function is already declared 'final' [modernize-use-override]
16    void ExpandPrivate(int pos, const SigningProvider& provider, FlatSigningProvider& out) const override final
17         ^                                                                                       ~~~~~~~~~
18src/script/descriptor.cpp:517:10: warning: 'override' is redundant since the function is already declared 'final' [modernize-use-override]
19    bool IsSolvable() const override final { return false; }
20         ^                  ~~~~~~~~~
21src/script/descriptor.cpp:542:10: warning: 'override' is redundant since the function is already declared 'final' [modernize-use-override]
22    bool IsSolvable() const override final { return false; }
23         ^                  ~~~~~~~~~

hebasto commented at 8:15 am on March 2, 2020:

This is final, but not virtual, so override is already implied. I am not sure why the compiler warns here?!

This GCC behavior fixed for 9.2 (GCC Bugzilla – Bug 78010).

hebasto commented at 9:56 am on March 2, 2020:

However clang-tidy with modernize-use-override will tell you that it, along with a bunch of the other overrides added here are redundant:

There is an AllowOverrideAndFinal option:

If set to non-zero, this check will not diagnose override as redundant with final. This is useful when code will be compiled by a compiler with warning/error checking flags requiring override explicitly on overridden members, such as gcc -Wsuggest-override/gcc -Werror=suggest-override. Default is 0.

hebasto commented at 8:44 pm on March 2, 2020:

Addressed in the latest push.

MarcoFalke commented at 2:52 pm on March 2, 2020: member

I am fine with the src/qt changes, but I am not sure if it make sense to “enforce” this with a warning that spits out misleading information on older and buggy compilers.

hebasto force-pushed on Mar 2, 2020

theuni commented at 8:13 pm on March 2, 2020: member

It may make sense to use a bunch of test-cases to test whether or not we should enable this, and include those cases that are known to cause issues with buggy compilers. Then just say “if your compiler can’t handle all test-cases correctly, we’ll just turn the warnings off wholesale”.

IIRC the c++xy m4 test macro takes a similar approach.

hebasto commented at 8:36 pm on March 2, 2020: member

Updated ec530398dbb3138d26c4783f880e4efe439fc450 -> f33cc5446b3d8358fa274cfee0287b30f82ceaab (pr16710.10 -> pr16710.11, compare):

removed ignoring of GCC “-Wsuggest-override” diagnostic for leveldb/env.h header as it is fixed in #17398
added ignoring of GCC “-Wsuggest-override” diagnostic for db_cxx.h header as it causes spammy warnings on Fedora 31 (GCC 9.2.1, libdb4-cxx-devel.x86_64::4.8.30-30.fc31)
addressed comments about final overriding functions

GCC 7.4.0 output:

 0$ gcc --version | head -1
 1gcc (Ubuntu 7.4.0-1ubuntu1~18.04.1) 7.4.0
 2$ make > /dev/null
 3/usr/bin/ar: `u' modifier ignored since `D' is the default (see `U')
 4ar: `u' modifier ignored since `D' is the default (see `U')
 5leveldb/db/c.cc: In function ‘leveldb_filterpolicy_t* leveldb_filterpolicy_create_bloom(int)’:
 6leveldb/db/c.cc:474:17: warning: ‘virtual const char* leveldb_filterpolicy_create_bloom(int)::Wrapper::Name() const’ can be marked override [-Wsuggest-override]
 7     const char* Name() const { return rep_->Name(); }
 8                 ^~~~
 9leveldb/db/c.cc:475:10: warning: ‘virtual void leveldb_filterpolicy_create_bloom(int)::Wrapper::CreateFilter(const leveldb::Slice*, int, std::__cxx11::string*) const’ can be marked override [-Wsuggest-override]
10     void CreateFilter(const Slice* keys, int n, std::string* dst) const {
11          ^~~~~~~~~~~~
12leveldb/db/c.cc:478:10: warning: ‘virtual bool leveldb_filterpolicy_create_bloom(int)::Wrapper::KeyMayMatch(const leveldb::Slice&, const leveldb::Slice&) const’ can be marked override [-Wsuggest-override]
13     bool KeyMayMatch(const Slice& key, const Slice& filter) const {
14          ^~~~~~~~~~~
15/usr/bin/ar: `u' modifier ignored since `D' is the default (see `U')

GCC 9.2.1 output:

 0$ gcc --version | head -1
 1gcc (GCC) 9.2.1 20190827 (Red Hat 9.2.1-1)
 2$ qmake-qt5 -v | grep Qt
 3Using Qt version 5.13.2 in /usr/lib64
 4$ make > /dev/null 
 5In file included from /usr/include/string.h:495,
 6                 from ./serialize.h:19,
 7                 from ./netaddress.h:13,
 8                 from ./protocol.h:13,
 9                 from protocol.cpp:6:
10In function ‘char* strncpy(char*, const char*, size_t)’,
11    inlined from ‘CMessageHeader::CMessageHeader(const unsigned char (&)[4], const char*, unsigned int)’ at protocol.cpp:89:12:
12/usr/include/bits/string_fortified.h:106:34: warning: ‘char* __builtin_strncpy(char*, const char*, long unsigned int)’ specified bound 12 equals destination size [-Wstringop-truncation]
13  106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
14      |          ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
15leveldb/db/c.cc: In function ‘leveldb_filterpolicy_t* leveldb_filterpolicy_create_bloom(int)’:
16leveldb/db/c.cc:474:17: warning: ‘virtual const char* leveldb_filterpolicy_create_bloom(int)::Wrapper::Name() const’ can be marked override [-Wsuggest-override]
17  474 |     const char* Name() const { return rep_->Name(); }
18      |                 ^~~~
19leveldb/db/c.cc:475:10: warning: ‘virtual void leveldb_filterpolicy_create_bloom(int)::Wrapper::CreateFilter(const leveldb::Slice*, int, std::string*) const’ can be marked override [-Wsuggest-override]
20  475 |     void CreateFilter(const Slice* keys, int n, std::string* dst) const {
21      |          ^~~~~~~~~~~~
22leveldb/db/c.cc:478:10: warning: ‘virtual bool leveldb_filterpolicy_create_bloom(int)::Wrapper::KeyMayMatch(const leveldb::Slice&, const leveldb::Slice&) const’ can be marked override [-Wsuggest-override]
23  478 |     bool KeyMayMatch(const Slice& key, const Slice& filter) const {
24      |          ^~~~~~~~~~~
25interfaces/chain.cpp: In member function ‘virtual std::unique_ptr<interfaces::Chain::Lock> interfaces::{anonymous}::ChainImpl::lock(bool)’:
26interfaces/chain.cpp:250:25: warning: redundant move in return statement [-Wredundant-move]
27  250 |         return std::move(result);
28      |                ~~~~~~~~~^~~~~~~~
29interfaces/chain.cpp:250:25: note: remove ‘std::move’ call

hebasto commented at 8:43 pm on March 2, 2020: member

@theuni

It may make sense to use a bunch of test-cases to test whether or not we should enable this, and include those cases that are known to cause issues with buggy compilers. Then just say “if your compiler can’t handle all test-cases correctly, we’ll just turn the warnings off wholesale”.

IIRC the c++xy m4 test macro takes a similar approach.

Yes, m4 macro could test if -Wsuggest-override is buggy for a particular GCC which is used for compiling.

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.

practicalswift commented at 8:45 pm on March 2, 2020: contributor

ACK f33cc5446b3d8358fa274cfee0287b30f82ceaab

hebasto commented at 8:50 pm on March 2, 2020: member

@MarcoFalke

I am fine with the src/qt changes, but I am not sure if it make sense to “enforce” this with a warning that spits out misleading information on older and buggy compilers.

If a false positive warning suppression approach seems too wordy, the last two commits could be dropped. In that case we will refuse benefits of -Wsuggest-override.

theuni commented at 10:12 pm on March 2, 2020: member

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

hebasto force-pushed on Mar 2, 2020

hebasto commented at 10:41 pm on March 2, 2020: member

Updated f33cc5446b3d8358fa274cfee0287b30f82ceaab -> d09c985ac4c7289875529da68f4a0ed1f04361f6 (pr16710.11 -> pr16710.12, compare):

fixed MSVC build

practicalswift commented at 11:25 pm on March 2, 2020: contributor

ACK d09c985ac4c7289875529da68f4a0ed1f04361f6

DrahtBot added the label Needs rebase on Mar 13, 2020

hebasto force-pushed on Mar 14, 2020

hebasto commented at 7:36 am on March 14, 2020: member

Rebased due to the conflict with #18204.

practicalswift commented at 8:20 am on March 14, 2020: contributor

ACK 2e9e8c8122c3c837e61f8b8b703a7807762ee550 – patch looks correct

DrahtBot removed the label Needs rebase on Mar 14, 2020

vasild commented at 10:26 am on April 24, 2020: member

ACK 2e9e8c812, compiles with Clang 11.0.0.

The more -W... the better :)

To clarify: Clang does not support the newly added flag, so there we properly don’t add it. It also compiles with gcc 9.3.0 where -Wsuggest-override is detected and used.

practicalswift commented at 12:50 pm on April 24, 2020: contributor

Reviewers of this PR might be interested in reviewing #17895 which enables Clang’s -Wconditional-uninitialized (warn on potential uninitialized reads).

DrahtBot added the label Needs rebase on May 6, 2020

hebasto force-pushed on May 8, 2020

hebasto commented at 7:41 am on May 8, 2020: member

Rebased due to the conflict with #18843.

DrahtBot removed the label Needs rebase on May 8, 2020

MarcoFalke commented at 10:58 am on May 8, 2020: member

What do you think about splitting out the gui changes from the build system changes? They are the bulk of the changes, but the easiest to review. Mostly move-only + adding the override, whereas the other changes are refactoring walletdb and changing the build stystem.

hebasto commented at 11:54 am on May 8, 2020: member

@MarcoFalke

What do you think about splitting out the gui changes from the build system changes? They are the bulk of the changes, but the easiest to review. Mostly move-only + adding the override, whereas the other changes are refactoring walletdb and changing the build stystem.

Done in #18914.

practicalswift commented at 6:11 am on May 9, 2020: contributor

ACK a96adef72a837c7988ee45990a69d9b15f0b7ef1

DrahtBot added the label Needs rebase on May 11, 2020

in configure.ac:388 in a96adef72a outdated

382@@ -383,6 +383,7 @@ if test "x$CXXFLAGS_overridden" = "xno"; then
383   AX_CHECK_COMPILE_FLAG([-Wunused-variable],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wunused-variable"],,[[$CXXFLAG_WERROR]])
384   AX_CHECK_COMPILE_FLAG([-Wdate-time],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wdate-time"],,[[$CXXFLAG_WERROR]])
385   AX_CHECK_COMPILE_FLAG([-Wconditional-uninitialized],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wconditional-uninitialized"],,[[$CXXFLAG_WERROR]])
386+  AX_CHECK_COMPILE_FLAG([-Wsuggest-override],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wsuggest-override"],,[[$CXXFLAG_WERROR]])

vasild commented at 3:20 pm on May 11, 2020:

Enabling -Wsuggest-override produces new warnings (with gcc 10):

0.../src/leveldb/db/c.cc: In function 'leveldb_filterpolicy_t* leveldb_filterpolicy_create_bloom(int)':
1.../src/leveldb/db/c.cc:474:17: warning: 'virtual const char* leveldb_filterpolicy_create_bloom(int)::Wrapper::Name() const' can be marked override [-Wsuggest-override]
2  474 |     const char* Name() const { return rep_->Name(); }
3      |                 ^~~~
4.../src/leveldb/db/c.cc:475:10: warning: 'virtual void leveldb_filterpolicy_create_bloom(int)::Wrapper::CreateFilter(const leveldb::Slice*, int, std::string*) const' can be marked override [-Wsuggest-override]
5  475 |     void CreateFilter(const Slice* keys, int n, std::string* dst) const {
6      |          ^~~~~~~~~~~~
7.../src/leveldb/db/c.cc:478:10: warning: 'virtual bool leveldb_filterpolicy_create_bloom(int)::Wrapper::KeyMayMatch(const leveldb::Slice&, const leveldb::Slice&) const' can be marked override [-Wsuggest-override]
8  478 |     bool KeyMayMatch(const Slice& key, const Slice& filter) const {
9      |          ^~~~~~~~~~~

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

 0diff --git i/configure.ac w/configure.ac
 1index 368541b0c..c35aa9343 100644
 2--- i/configure.ac
 3+++ w/configure.ac
 4@@ -364,12 +364,13 @@ if test "x$enable_werror" = "xyes"; then
 5   AX_CHECK_COMPILE_FLAG([-Werror=switch],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=switch"],,[[$CXXFLAG_WERROR]])
 6   AX_CHECK_COMPILE_FLAG([-Werror=thread-safety-analysis],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=thread-safety-analysis"],,[[$CXXFLAG_WERROR]])
 7   AX_CHECK_COMPILE_FLAG([-Werror=unused-variable],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=unused-variable"],,[[$CXXFLAG_WERROR]])
 8   AX_CHECK_COMPILE_FLAG([-Werror=date-time],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=date-time"],,[[$CXXFLAG_WERROR]])
 9   AX_CHECK_COMPILE_FLAG([-Werror=return-type],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=return-type"],,[[$CXXFLAG_WERROR]])
10   AX_CHECK_COMPILE_FLAG([-Werror=conditional-uninitialized],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=conditional-uninitialized"],,[[$CXXFLAG_WERROR]])
11+  AX_CHECK_COMPILE_FLAG([-Werror=suggest-override],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=suggest-override"],,[[$CXXFLAG_WERROR]])
12 fi
13 
14 if test "x$CXXFLAGS_overridden" = "xno"; then
15   AX_CHECK_COMPILE_FLAG([-Wall],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wall"],,[[$CXXFLAG_WERROR]])
16   AX_CHECK_COMPILE_FLAG([-Wextra],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wextra"],,[[$CXXFLAG_WERROR]])
17   AX_CHECK_COMPILE_FLAG([-Wgnu],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wgnu"],,[[$CXXFLAG_WERROR]])
18diff --git i/src/Makefile.leveldb.include w/src/Makefile.leveldb.include
19index 79ff72ca8..8a28f4f24 100644
20--- i/src/Makefile.leveldb.include
21+++ w/src/Makefile.leveldb.include
22@@ -33,13 +33,13 @@ if TARGET_WINDOWS
23 LEVELDB_CPPFLAGS_INT += -DLEVELDB_PLATFORM_WINDOWS -D_UNICODE -DUNICODE -D__USE_MINGW_ANSI_STDIO=1
24 else
25 LEVELDB_CPPFLAGS_INT += -DLEVELDB_PLATFORM_POSIX
26 endif
27 
28 leveldb_libleveldb_a_CPPFLAGS = $(AM_CPPFLAGS) $(LEVELDB_CPPFLAGS_INT) $(LEVELDB_CPPFLAGS)
29-leveldb_libleveldb_a_CXXFLAGS = $(filter-out -Wconditional-uninitialized -Werror=conditional-uninitialized, $(AM_CXXFLAGS)) $(PIE_FLAGS)
30+leveldb_libleveldb_a_CXXFLAGS = $(filter-out -Wconditional-uninitialized -Werror=conditional-uninitialized -Wsuggest-override -Werror=suggest-override, $(AM_CXXFLAGS)) $(PIE_FLAGS)
31 
32 leveldb_libleveldb_a_SOURCES=
33 leveldb_libleveldb_a_SOURCES += leveldb/port/port_stdcxx.h
34 leveldb_libleveldb_a_SOURCES += leveldb/port/port.h
35 leveldb_libleveldb_a_SOURCES += leveldb/port/thread_annotations.h
36 leveldb_libleveldb_a_SOURCES += leveldb/include/leveldb/db.h

hebasto commented at 6:48 pm on May 11, 2020:

Updated.

vasild commented at 3:23 pm on May 11, 2020: member

Approach ACK

MarcoFalke referenced this in commit eb2ffbb7c1 on May 11, 2020

refactor: Add BerkeleyDatabaseVersion() function de5e91c303

hebasto force-pushed on May 11, 2020

DrahtBot removed the label Needs rebase on May 11, 2020

hebasto commented at 6:47 pm on May 11, 2020: member

Updated a96adef72a837c7988ee45990a69d9b15f0b7ef1 -> d44a13b4b2dac522e5f68ca5bdd9cd972ae6d5e9 (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).

MarcoFalke commented at 7:27 pm on May 11, 2020: member

I don’t think d44a13b4b2dac522e5f68ca5bdd9cd972ae6d5e9 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 commented at 7:28 pm on May 11, 2020: member

ACK d44a13b4b

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 force-pushed on May 12, 2020

hebasto commented at 9:10 am on May 12, 2020: member

Updated d44a13b4b2dac522e5f68ca5bdd9cd972ae6d5e9 -> ea6849fe2fb53bfd50e3805b45746a36208b0781 (pr16710.15 -> pr16710.16, diff):

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

hebasto commented at 9:13 am on May 12, 2020: member

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

in configure.ac:360 in ea6849fe2f outdated

354@@ -355,6 +355,24 @@ if test x$use_sanitizers != x; then
355     ]],[[]])])
356 fi
357 
358+dnl GCC<9.2 reports a redundant -Wsuggest-override warning on a 'final' method.
359+dnl See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78010
360+AC_MSG_CHECKING(for GCC compiler version < 9.2)

vasild commented at 9:49 am on May 12, 2020:

nit: just “… GCC version …” is ok too (remove “compiler” because “GCC” means “GNU Compiler Collection”)

hebasto commented at 10:08 am on May 12, 2020:

Updated.

in configure.ac:390 in ea6849fe2f outdated

385@@ -368,6 +386,9 @@ if test "x$enable_werror" = "xyes"; then
386   AX_CHECK_COMPILE_FLAG([-Werror=return-type],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=return-type"],,[[$CXXFLAG_WERROR]])
387   AX_CHECK_COMPILE_FLAG([-Werror=conditional-uninitialized],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=conditional-uninitialized"],,[[$CXXFLAG_WERROR]])
388   AX_CHECK_COMPILE_FLAG([-Werror=sign-compare],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=sign-compare"],,[[$CXXFLAG_WERROR]])
389+  if test "x$have_gcc_suggest_override_bug" = "xno"; then
390+    AX_CHECK_COMPILE_FLAG([-Werror=suggest-override],[WARN_CXXFLAGS="$ERROR_CXXFLAGS -Werror=suggest-override"],,[[$CXXFLAG_WERROR]])

vasild commented at 9:52 am on May 12, 2020:

s/WARN_CXXFLAGS/ERROR_CXXFLAGS/

hebasto commented at 10:08 am on May 12, 2020:

Updated.

hebasto force-pushed on May 12, 2020

hebasto commented at 10:08 am on May 12, 2020: member

Updated ea6849fe2fb53bfd50e3805b45746a36208b0781 -> e6211d8b54338418575ffb762860c7634e10c977 (pr16710.16 -> pr16710.17, diff):

addressed @vasild’s comments

vasild approved

vasild commented at 10:36 am on May 12, 2020: member

ACK e6211d8b5

GCC 8.4.0:

0checking for GCC version < 9.2... yes
1configure: 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:

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

Clang 11.0.0:

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

in configure.ac:372 in e6211d8b54 outdated

367+    AC_MSG_RESULT(no)
368+    have_gcc_suggest_override_bug=no
369+  ],
370+  [
371+    AC_MSG_RESULT(yes)
372+    AC_MSG_WARN([GCC<9.2 reports a redundant -Wsuggest-override warning on a 'final' method, disabling -Wsuggest-override.])

fanquake commented at 1:17 pm on May 12, 2020:

If we do keep this as is, let’s drop this call to AC_MSG_WARN and the AC_MSG_CHECKING above. I don’t think we need any extra, verbose out here. Especially since this will be the default case most people compiling going forward.

hebasto commented at 3:10 pm on May 12, 2020:

Updated.

fanquake changes_requested

fanquake commented at 1:29 pm on May 12, 2020: member

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

 0diff --git a/configure.ac b/configure.ac
 1index 7de6adb93..31e9c243f 100644
 2--- a/configure.ac
 3+++ b/configure.ac
 4@@ -368,6 +368,11 @@ if test "x$enable_werror" = "xyes"; then
 5   AX_CHECK_COMPILE_FLAG([-Werror=return-type],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=return-type"],,[[$CXXFLAG_WERROR]])
 6   AX_CHECK_COMPILE_FLAG([-Werror=conditional-uninitialized],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=conditional-uninitialized"],,[[$CXXFLAG_WERROR]])
 7   AX_CHECK_COMPILE_FLAG([-Werror=sign-compare],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=sign-compare"],,[[$CXXFLAG_WERROR]])
 8+
 9+  dnl -Wsuggest-override is  broken with GCC before 9.2
10+  dnl https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78010
11+  AX_CHECK_COMPILE_FLAG([-Wsuggest-override],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wsuggest-override"],,[[$CXXFLAG_WERROR]],
12+                       [AC_LANG_PROGRAM([[struct A { virtual void f(); };struct B : A { void f() final; };]])])
13 fi
14 
15 if test "x$CXXFLAGS_overridden" = "xno"; then
16@@ -385,6 +390,8 @@ if test "x$CXXFLAGS_overridden" = "xno"; then
17   AX_CHECK_COMPILE_FLAG([-Wdate-time],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wdate-time"],,[[$CXXFLAG_WERROR]])
18   AX_CHECK_COMPILE_FLAG([-Wconditional-uninitialized],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wconditional-uninitialized"],,[[$CXXFLAG_WERROR]])
19   AX_CHECK_COMPILE_FLAG([-Wsign-compare],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wsign-compare"],,[[$CXXFLAG_WERROR]])
20+  AX_CHECK_COMPILE_FLAG([-Wsuggest-override],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wsuggest-override"],,[[$CXXFLAG_WERROR]],
21+                       [AC_LANG_PROGRAM([[struct A { virtual void f(); };struct B : A { void f() final; };]])])
22 
23   dnl Some compilers (gcc) ignore unknown -Wno-* options, but warn about all
24   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.

build: Enable -Wsuggest-override 839add193b

hebasto force-pushed on May 12, 2020

hebasto commented at 3:09 pm on May 12, 2020: member

@fanquake Thank you for your review. TIL :)

Updated e6211d8b54338418575ffb762860c7634e10c977 -> 839add193b13c17a40f42ff69d973caeb800d3f2 (pr16710.17 -> pr16710.18, diff).

addressed @fanquake’s comment

practicalswift commented at 3:11 pm on May 12, 2020: contributor

ACK 839add193b13c17a40f42ff69d973caeb800d3f2 assuming Travis is happy: patch looks correct

vasild commented at 3:31 pm on May 12, 2020: member

ACK 839add193

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

fanquake approved

fanquake commented at 7:15 am on May 13, 2020: member

ACK 839add193b13c17a40f42ff69d973caeb800d3f2

fanquake merged this on May 13, 2020

fanquake closed this on May 13, 2020

hebasto deleted the branch on May 13, 2020

sidhujag referenced this in commit 4f62a6d3b7 on May 14, 2020

Fabcien referenced this in commit 4770199b7a on Jan 28, 2021

kittywhiskers referenced this in commit 6dd90eaacc on Jun 16, 2021

kittywhiskers referenced this in commit 93b76fb097 on Jun 17, 2021

kittywhiskers referenced this in commit 31e192f9f6 on Jun 17, 2021

kittywhiskers referenced this in commit 4802b41819 on Jun 17, 2021

kittywhiskers referenced this in commit b07abef88a on Jun 17, 2021

UdjinM6 referenced this in commit 988fa6a235 on Jun 23, 2021

Fabcien referenced this in commit 85aad14465 on Sep 29, 2021

DrahtBot locked this on Feb 15, 2022

build: Enable -Wsuggest-override if available #16710

Conflicts