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
Dedup IsFileSectorAligned() to fix unity build. #5812
Conversation
Summary: Unity build fails because of name conflict of IsFileSectorAligned() after recent refactoring. Consolidate the function. Test Plan: make unity. At least the failure goes away. Also "make all", "make release" and see no regression in normal cases.
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 with a couple of minor comments. Thanks!
file/read_write_util.h
Outdated
@@ -26,4 +26,9 @@ extern Status NewWritableFile(Env* env, const std::string& fname, | |||
bool ReadOneLine(std::istringstream* iss, SequentialFile* seq_file, | |||
std::string* output, bool* has_data, Status* result); | |||
|
|||
#ifndef NDEBUG | |||
bool IsFileSectorAligned(const size_t off, size_t sector_size) { |
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 think we should mark this inline if we're going to put the implementation in the header file (alternatively, the definition could be moved to the .cc).
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.
Yes, I noticed it too. I moved it to .cc file now.
@@ -15,15 +15,6 @@ | |||
#include "util/rate_limiter.h" | |||
|
|||
namespace rocksdb { | |||
|
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.
#include "file/read_write_util.h"
in this file as well?
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.
Fixed. Good catch.
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.
@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
This pull request has been merged in 811e403. |
Summary: Unity build fails because of name conflict of IsFileSectorAligned() after recent refactoring. Consolidate the function. Pull Request resolved: facebook#5812 Test Plan: make unity. At least the failure goes away. Also "make all", "make release" and see no regression in normal cases. Differential Revision: D17411403 fbshipit-source-id: 09d5653471ae2c3a4d898e120a024f7dd08d9c9d
Summary: Unity build fails because of name conflict of IsFileSectorAligned() after recent refactoring. Consolidate the function. Pull Request resolved: facebook/rocksdb#5812 Test Plan: make unity. At least the failure goes away. Also "make all", "make release" and see no regression in normal cases. Differential Revision: D17411403 fbshipit-source-id: 09d5653471ae2c3a4d898e120a024f7dd08d9c9d Signed-off-by: Changlong Chen <levisonchen@live.cn>
Summary: Unity build fails because of name conflict of IsFileSectorAligned() after recent refactoring. Consolidate the function. Pull Request resolved: facebook/rocksdb#5812 Test Plan: make unity. At least the failure goes away. Also "make all", "make release" and see no regression in normal cases. Differential Revision: D17411403 fbshipit-source-id: 09d5653471ae2c3a4d898e120a024f7dd08d9c9d Signed-off-by: Changlong Chen <levisonchen@live.cn>
Summary:
Unity build fails because of name conflict of IsFileSectorAligned() after recent refactoring. Consolidate the function.
Test Plan: make unity. At least the failure goes away. Also "make all", "make release" and see no regression in normal cases.