remove redundant std::move #8478
Conversation
rockeet
force-pushed
the
remove-redundant-stdmove
branch
from
a774244
to
f2a9e89
Compare
It should be expressed explicitly, I've fixed it |
|
@ajkr you may take a look? |
|
Doesn't matter to me. It appears to do the same work in a slightly more roundabout and slightly less suspicious-looking way. |
| #ifdef ROCKSDB_ASSERT_STATUS_CHECKED | ||
| mutable bool checked_ = false; | ||
| #endif // ROCKSDB_ASSERT_STATUS_CHECKED | ||
| const char* state_; |
There was a problem hiding this comment.
Why we move the line here?
There was a problem hiding this comment.
move the line here eliminate paddings and reduces sizeof(Status) by 8 bytes when ROCKSDB_ASSERT_STATUS_CHECKED is defined.
|
@zhichao-cao has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
| @@ -266,4 +266,8 @@ inline IOStatus status_to_io_status(Status&& status) { | |||
| } | |||
| } | |||
|
|
|||
| inline Status io_status_to_status(IOStatus& io_s) { | |||
There was a problem hiding this comment.
@anand1976 Are you ok this this change?
There was a problem hiding this comment.
Seems ok to me. Can we make it io_statuc_to_status(IOStatus&& io_s)? I think doing std::move on a lvalue reference is a bit of an anti-pattern.
There was a problem hiding this comment.
This seems unnecessary (and violating Google C++ style on names). What's wrong with
Status(io_s)
or
static_cast<const Status&>(io_s);
? If you're going to motivate use of move semantics, I wouldn't use a case where it's eligible for copy elision. https://en.cppreference.com/w/cpp/language/copy_elision
| @@ -266,4 +266,8 @@ inline IOStatus status_to_io_status(Status&& status) { | |||
| } | |||
| } | |||
|
|
|||
| inline Status io_status_to_status(IOStatus& io_s) { | |||
There was a problem hiding this comment.
Seems ok to me. Can we make it io_statuc_to_status(IOStatus&& io_s)? I think doing std::move on a lvalue reference is a bit of an anti-pattern.
db/db_impl/db_impl.cc
Outdated
| @@ -1348,7 +1348,7 @@ Status DBImpl::LockWAL() { | |||
| // future writes | |||
| WriteStatusCheck(status); | |||
| } | |||
| return std::move(status); | |||
| return io_status_to_status(status); | |||
There was a problem hiding this comment.
Can we make the type of status explicit in line 1343?
rockeet
force-pushed
the
remove-redundant-stdmove
branch
from
f2a9e89
to
08323d2
Compare
|
@rockeet has updated the pull request. You must reimport the pull request before landing. |
No description provided.