Skip to content

cmake: Silent leveldb-specific warnings #99

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Feb 15, 2024
Merged

cmake: Silent leveldb-specific warnings #99

merged 1 commit into from
Feb 15, 2024

Conversation

hebasto
Copy link
Owner

@hebasto hebasto commented Feb 9, 2024

This PR split from #84

It introduces a new warn_leveldb_interface, which overrides -Wconditional-uninitialized and -Wsuggest-override if any.

For MSVC builds, dropped warning suppressions that are provided at the global level.

For example, on the staging branch:

$ env CC=clang CXX=clang++ CXXFLAGS="-Wconditional-uninitialized -Wsuggest-override" cmake -B build
$ cmake --build build --target leveldb > /dev/null 
/home/hebasto/git/bitcoin/src/leveldb/db/c.cc:474:17: warning: 'Name' overrides a member function but is not marked 'override' [-Wsuggest-override]
    const char* Name() const { return rep_->Name(); }
                ^
/home/hebasto/git/bitcoin/src/leveldb/db/c.cc:110:15: note: overridden virtual function is here
  const char* Name() const override { return (*name_)(state_); }
              ^
/home/hebasto/git/bitcoin/src/leveldb/db/c.cc:475:10: warning: 'CreateFilter' overrides a member function but is not marked 'override' [-Wsuggest-override]
    void CreateFilter(const Slice* keys, int n, std::string* dst) const {
         ^
/home/hebasto/git/bitcoin/src/leveldb/db/c.cc:112:8: note: overridden virtual function is here
  void CreateFilter(const Slice* keys, int n, std::string* dst) const override {
       ^
/home/hebasto/git/bitcoin/src/leveldb/db/c.cc:478:10: warning: 'KeyMayMatch' overrides a member function but is not marked 'override' [-Wsuggest-override]
    bool KeyMayMatch(const Slice& key, const Slice& filter) const {
         ^
/home/hebasto/git/bitcoin/src/leveldb/db/c.cc:125:8: note: overridden virtual function is here
  bool KeyMayMatch(const Slice& key, const Slice& filter) const override {
       ^
3 warnings generated.
/home/hebasto/git/bitcoin/src/leveldb/db/version_set.cc:1016:55: warning: variable 'manifest_size' may be uninitialized when used here [-Wconditional-uninitialized]
  descriptor_log_ = new log::Writer(descriptor_file_, manifest_size);
                                                      ^~~~~~~~~~~~~
/home/hebasto/git/bitcoin/src/leveldb/db/version_set.cc:997:25: note: initialize the variable 'manifest_size' to silence this warning
  uint64_t manifest_size;
                        ^
                         = 0
1 warning generated.

@hebasto
Copy link
Owner Author

hebasto commented Feb 9, 2024

Friendly ping @fanquake @TheCharlatan @m3dwards @pablomartin4btc @vasild :)

@theuni Your https://github.com/hebasto/bitcoin/pull/6/files#r1113411466 has been addressed.

@hebasto hebasto mentioned this pull request Feb 9, 2024
Merged
@hebasto hebasto added the enhancement New feature or request label Feb 10, 2024
@theuni
Copy link

theuni commented Feb 12, 2024

Concept ACK. I think "nowarn_leveldb_interface" would make this more intuitive though.

@hebasto
fixup! cmake: Build leveldb static library
33dee19 
This change introduces a new `warn_leveldb_interface`, which overrides
`-Wconditional-uninitialized` and `-Wsuggest-override` if any.

For MSVC builds, dropped warning suppressions that are provided at the
global level.
@hebasto
Copy link
Owner Author

hebasto commented Feb 12, 2024

I think "nowarn_leveldb_interface" would make this more intuitive though.

Thanks! Renamed.

vasild
vasild approved these changes Feb 13, 2024
View reviewed changes
Copy link

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 33dee19

theuni
theuni reviewed Feb 13, 2024
View reviewed changes
cmake/leveldb.cmake
/wd4244
/wd4267
$<$<CONFIG:Release>:/wd4722>
target_compile_options(nowarn_leveldb_interface INTERFACE
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these no longer private?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only INTERFACE_* properties might be populated for interface libraries.

The nowarn_leveldb_interface library itself is linked in the PRIVATE scope though.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise, CMake raises an error:

CMake Error at cmake/leveldb.cmake:86 (target_compile_options):
  target_compile_options may only set INTERFACE properties on INTERFACE
  targets
Call Stack (most recent call first):
  CMakeLists.txt:260 (include)

theuni
theuni approved these changes Feb 14, 2024
View reviewed changes
Copy link

@theuni theuni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 33dee19

@hebasto hebasto merged commit 2082e3d into cmake-staging Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
2 more reviewers

@vasild vasild vasild approved these changes

@theuni theuni theuni approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hebasto @theuni @vasild