refactor: add missing overrides #51
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes instances of:
```bash
leveldb-subtree/db/log_test.cc:208:17: error: 'GetName' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]
208 | std::string GetName() const { return ""; }
```
laanwj
approved these changes
May 8, 2025
|
Is there a related upstream commit or issue? |
I don't think so. Upstream still uses C++11, and I don't think they would take this change. |
hebasto
approved these changes
May 8, 2025
There was a problem hiding this comment.
ACK 9defe84, tested on Ubuntu 25.04 with GCC 14.2.0 and Clang 20.1.2.
Additionally tested with the Bitcoin Core code base being patched with:
--- a/cmake/leveldb.cmake
+++ b/cmake/leveldb.cmake
@@ -90,9 +90,6 @@ else()
try_append_cxx_flags("-Wconditional-uninitialized" TARGET nowarn_leveldb_interface SKIP_LINK
IF_CHECK_PASSED "-Wno-conditional-uninitialized"
)
- try_append_cxx_flags("-Wsuggest-override" TARGET nowarn_leveldb_interface SKIP_LINK
- IF_CHECK_PASSED "-Wno-suggest-override"
- )
endif()
target_link_libraries(leveldb PRIVATEI also have to notice that compiling fails without this patch:
--- a/benchmarks/db_bench_sqlite3.cc
+++ b/benchmarks/db_bench_sqlite3.cc
@@ -5,6 +5,7 @@
#include <sqlite3.h>
#include <stdio.h>
#include <stdlib.h>
+#include <ctime>
#include "util/histogram.h"
#include "util/random.h"
It's worse actually, it uses C++17 syntax that doesn't even compile currently - google#1251 |
fanquake
added a commit
to fanquake/bitcoin
that referenced
this pull request
May 13, 2025
113db4962b Merge bitcoin-core/leveldb-subtree#51: refactor: add missing overrides 9defe8494a refactor: add missing overrides git-subtree-dir: src/leveldb git-subtree-split: 113db4962b8be416c17ad2b4bdcc5514c51d7181
fanquake
added a commit
to fanquake/bitcoin
that referenced
this pull request
May 16, 2025
113db4962b Merge bitcoin-core/leveldb-subtree#51: refactor: add missing overrides 9defe8494a refactor: add missing overrides git-subtree-dir: src/leveldb git-subtree-split: 113db4962b8be416c17ad2b4bdcc5514c51d7181
hebasto
added a commit
to bitcoin/bitcoin
that referenced
this pull request
May 17, 2025
7015052 build: remove Wsuggest-override suppression from leveldb build (fanquake) e2c84b8 Squashed 'src/leveldb/' changes from 4188247086..113db4962b (fanquake) Pull request description: Pulls in * bitcoin-core/leveldb-subtree#51 Remove the related warning suppression. ACKs for top commit: l0rinc: utACK 7015052 hebasto: ACK 7015052, I've updated the `leveldb` subtree locally and got zero diff with this branch. Tree-SHA512: 1ac7c8ecc9025086b429e12c22fc25f654eaf68fc9500b95341fb635cea12e7f80d69298cff120e8557a4f2f5809956a3b158cdb4db745cfa605c0df6f346423
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes instances of:
To fix compile failures under C++20, and removing warning supression in the main repo.