-
Notifications
You must be signed in to change notification settings - Fork 95
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 (facebook#9374) #271
Conversation
There is still one error:
|
options/db_options.h
Outdated
@@ -89,6 +89,7 @@ struct MutableDBOptions { | |||
MutableDBOptions(); | |||
explicit MutableDBOptions(const MutableDBOptions& options) = default; | |||
explicit MutableDBOptions(const DBOptions& options); | |||
MutableDBOptions& operator=(const MutableDBOptions& options) = default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So why defining a default copy constructor in the first place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, my fix here is not correct. I should've removed the uses of implicit assignment instead.
The explicit MutableDBOptions(const MutableDBOptions& options)
is probably here to make creating copy of DB's mutable options less convenient, so we don't end up with lots of options with differing values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, the size_t valid_batches = 0;
error is gone in the latest code.
options/db_options.h
Outdated
@@ -89,6 +89,7 @@ struct MutableDBOptions { | |||
MutableDBOptions(); | |||
explicit MutableDBOptions(const MutableDBOptions& options) = default; | |||
explicit MutableDBOptions(const DBOptions& options); | |||
MutableDBOptions& operator=(MutableDBOptions&& options) = default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I download the latest rocksdb code and can build it without any errors. It seems they delete the explicit copy constructor instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commit that introduces the change: facebook@9c6fb26
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll cherry-pick it instead.
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: tabokie xy.tao@outlook.com
Fix tikv/tikv#12204