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

Fix clang13 build error #9374

Closed
wants to merge 6 commits into from
Closed

Conversation

jay-zhuang
Copy link
Contributor

Test Plan: Add CI for clang13 build

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.

LGTM as long as CI build passes. Left a couple of minor questions.

@@ -27,7 +27,7 @@ class SequentialFileMirror : public SequentialFile {
if (as == Status::OK()) {
char* bscratch = new char[n];
Slice bslice;
size_t off = 0;
size_t off __attribute__((__unused__)) = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used in assert(). So the compiler only complain when DEBUG_LEVEL=0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Windows build is failing for that. How about we only define that with in #ifndef NDEBUG?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think attribute is gcc/clang thing

@@ -116,7 +116,6 @@ struct ImmutableDBOptions {
struct MutableDBOptions {
static const char* kName() { return "MutableDBOptions"; }
MutableDBOptions();
explicit MutableDBOptions(const MutableDBOptions& options) = default;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the error related to this copy constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the error message:

./options/db_options.h:119:12: error: definition of implicit copy assignment operator for 'MutableDBOptions' is deprecated because it has a user-declared copy constructor [-Werror,-Wdeprecated-copy]
  explicit MutableDBOptions(const MutableDBOptions& options) = default;
           ^
db/db_impl/db_impl.cc:1229:27: note: in implicit copy assignment operator for 'rocksdb::MutableDBOptions' first required here
      mutable_db_options_ = new_options;

@@ -2838,8 +2837,6 @@ TEST_F(DBTest2, ReadAmpBitmapLiveInCacheAfterDBClose) {

if (read_keys.find(i) == read_keys.end()) {
auto internal_key = InternalKey(key, 0, ValueType::kTypeValue);
total_useful_bytes +=
Copy link
Contributor

Choose a reason for hiding this comment

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

Does clang-13 think this var is not used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think it is set but never used.

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

tabokie pushed a commit to tabokie/rocksdb that referenced this pull request Mar 29, 2022
Summary: Pull Request resolved: facebook#9374

Test Plan: Add CI for clang13 build

Reviewed By: riversand963

Differential Revision: D33522867

Pulled By: jay-zhuang

fbshipit-source-id: 642756825cf0b51e35861fb847ebaee4611b76ca
Signed-off-by: tabokie <xy.tao@outlook.com>
tabokie added a commit to tikv/rocksdb that referenced this pull request Mar 29, 2022
Summary: Pull Request resolved: facebook#9374

Test Plan: Add CI for clang13 build

Reviewed By: riversand963

Differential Revision: D33522867

Pulled By: jay-zhuang

fbshipit-source-id: 642756825cf0b51e35861fb847ebaee4611b76ca
Signed-off-by: tabokie <xy.tao@outlook.com>

Co-authored-by: Jay Zhuang <zjay@fb.com>
Myasuka pushed a commit to Myasuka/rocksdb that referenced this pull request Jul 18, 2022
Summary: Pull Request resolved: facebook#9374

Test Plan: Add CI for clang13 build

Reviewed By: riversand963

Differential Revision: D33522867

Pulled By: jay-zhuang

fbshipit-source-id: 642756825cf0b51e35861fb847ebaee4611b76ca
Myasuka pushed a commit to Myasuka/frocksdb that referenced this pull request Jul 18, 2022
Summary: Pull Request resolved: facebook/rocksdb#9374

Test Plan: Add CI for clang13 build

Reviewed By: riversand963

Differential Revision: D33522867

Pulled By: jay-zhuang

fbshipit-source-id: 642756825cf0b51e35861fb847ebaee4611b76ca
Myasuka pushed a commit to Myasuka/frocksdb that referenced this pull request Jul 18, 2022
Summary: Pull Request resolved: facebook/rocksdb#9374

Test Plan: Add CI for clang13 build

Reviewed By: riversand963

Differential Revision: D33522867

Pulled By: jay-zhuang

fbshipit-source-id: 642756825cf0b51e35861fb847ebaee4611b76ca
fredia pushed a commit to fredia/frocksdb that referenced this pull request Nov 21, 2022
Summary: Pull Request resolved: facebook/rocksdb#9374

Test Plan: Add CI for clang13 build

Reviewed By: riversand963

Differential Revision: D33522867

Pulled By: jay-zhuang

fbshipit-source-id: 642756825cf0b51e35861fb847ebaee4611b76ca
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

3 participants