Skip to content
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.

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

gcc-11 and cmake related cleanup #9286

Closed
wants to merge 3 commits into from
Closed

Conversation

tchaikov
Copy link
Contributor

@tchaikov tchaikov commented Dec 12, 2021

in hope to get rockdb compiled with GCC-11 without warning

  • util/bloom_test: init a variable before using it
    to silence the GCC warning like
    util/bloom_test.cc:1253:31: error: ‘<anonymous>’ may be used uninitialized [-Werror=maybe-uninitialized]
     1253 |   Slice key_slice{key_bytes, 8};
          |                               ^
    ...
    include/rocksdb/slice.h:41:3: note: by argument 2 of type ‘const char*’ to ‘rocksdb::Slice::Slice(const char*, size_t)’ declared here
       41 |   Slice(const char* d, size_t n) : data_(d), size_(n) {}
          |   ^~~~~
    util/bloom_test.cc:1249:3: note: ‘<anonymous>’ declared here
     1249 |   };
          |   ^
    cc1plus: all warnings being treated as errors
    
  • cmake: add find_package(uring ...)
    find liburing in a more consistent way. also it is the encouraged way for finding a library.

Signed-off-by: Kefu Chai <tchaikov@gmail.com>
to silence the GCC warning like

util/bloom_test.cc:1253:31: error: ‘<anonymous>’ may be used uninitialized [-Werror=maybe-uninitialized]
 1253 |   Slice key_slice{key_bytes, 8};
      |                               ^
...
include/rocksdb/slice.h:41:3: note: by argument 2 of type ‘const char*’ to ‘rocksdb::Slice::Slice(const char*, size_t)’ declared here
   41 |   Slice(const char* d, size_t n) : data_(d), size_(n) {}
      |   ^~~~~
util/bloom_test.cc:1249:3: note: ‘<anonymous>’ declared here
 1249 |   };
      |   ^
cc1plus: all warnings being treated as errors

Signed-off-by: Kefu Chai <tchaikov@gmail.com>
find liburing in a more consistent way.

Signed-off-by: Kefu Chai <tchaikov@gmail.com>
Copy link
Contributor

@mrambacher mrambacher left a comment

Choose a reason for hiding this comment

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

This change generally LGTM.

Can you please put more information into the description/comment as to the exact problems/errors/messages you were seeing (at least for the -Werror and liburing cases)? This might help others who see those errors when searching for comparables.

Comment on lines +355 to +358
if (NOT BUILTIN_ATOMIC)
#TODO: Check if -latomic exists
list(APPEND THIRDPARTY_LIBS atomic)
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think I understand the indent but it looks like tabs instead of spaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

double checked. they are spaces.

CMakeLists.txt Outdated Show resolved Hide resolved
@tchaikov
Copy link
Contributor Author

tchaikov commented Dec 15, 2021

This change generally LGTM.

Can you please put more information into the description/comment as to the exact problems/errors/messages you were seeing (at least for the -Werror and liburing cases)? This might help others who see those errors when searching for comparables.

hi @mrambacher , i think i did put the detailed error messages in the commit messages . regarding to the liburing change, i am trying to use the modern CMake way to find and expose a library. if you want to have more background on this. i'd happy to provide more reading materials.

Copy link
Contributor

@jay-zhuang jay-zhuang left a comment

Choose a reason for hiding this comment

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

LGTM

@facebook-github-bot
Copy link
Contributor

@jay-zhuang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Contributor

@riversand963 riversand963 left a comment

Choose a reason for hiding this comment

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

Thanks @tchaikov for the PR.

i think i did put the detailed error messages in the commit messages

Could you update PR description? The individual commit message of this PR will be lost once merged to main, and the PR description will be the final commit message.

@tchaikov
Copy link
Contributor Author

Thanks @tchaikov for the PR.

i think i did put the detailed error messages in the commit messages

Could you update PR description? The individual commit message of this PR will be lost once merged to main, and the PR description will be the final commit message.

sure, done. thank you!

@mrambacher i must have misunderstood what you suggested. just realized that i should have updated the PR description. sorry and thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants