Repository navigation
Conversation
Drop single point of return rule. Functions complexity is decreased by allowing an early return.
License Check Results🚀 The license check job ran with the Bazel command: bazel run --lockfile_mode=error //:license-checkStatus: Click to expand output |
|
Documentation preview for this pull request is available at: |
| kvs.logger_->LogInfo() << "[instance=" << instance_id.id << "]" | ||
| << "[snapshot=" << snapshot_id.id << "]" | ||
| << "[maxSnapshotCount=" << kvs.max_snapshots_ << "] KVS opened"; | ||
| return kvs; |
There was a problem hiding this comment.
this function should return score::Result now it is returning kvs but the issue here kvs copy operations is deleted because it contains unique pointers .
please reuturn std::move(kvs);
There was a problem hiding this comment.
This is implicit: https://stackoverflow.com/a/9532647
GCC even has flags to detect unnecessary std::move: https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/C_002b_002b-Dialect-Options.html search for pessimizing-move and redundant-move.
There was a problem hiding this comment.
in this case kvs is not copy elision or not NRVO(Named Return Value optimization) because function return Result not kvs . but it working in this case because return initalization: compiler first tries to apply move and same time Result's constructor accept T&& and using std::forward() so make R-Value conversion
tested when i trued to enforce return fuction to return Lvalue then i got error :
in summary , you do not need to fix it for now .
| } | ||
|
|
||
| return result; | ||
| return result_value; |
There was a problem hiding this comment.
here unordered_map copyable but to avoid silently deep-copy i would recommed to return using std::move(result_value)
There was a problem hiding this comment.
This is implicit: https://stackoverflow.com/a/9532647
GCC even has flags to detect unnecessary std::move: https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/C_002b_002b-Dialect-Options.html search for pessimizing-move and redundant-move.
| } | ||
|
|
||
| return result; | ||
| return keys; |
There was a problem hiding this comment.
here vector is also copyable but to avoid silently deep-copy i would recommed to return using std::move(result_value)
There was a problem hiding this comment.
This is implicit: https://stackoverflow.com/a/9532647
GCC even has flags to detect unnecessary std::move: https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/C_002b_002b-Dialect-Options.html search for pessimizing-move and redundant-move.
| } | ||
| logger_->LogError() << "Error: Could not get size of file" << file_path << ":" << ec.message(); | ||
| return score::MakeUnexpected(ErrorCode::PhysicalStorageFailure); | ||
| else |
There was a problem hiding this comment.
[minor] un necessary else , we can remove it
There was a problem hiding this comment.
I found keeping else if and else in some places to be somewhat more readable, personal preference, fix unnecessary.
| } | ||
| return KvsValue(static_cast<int32_t>(n.value())); | ||
| } | ||
| else if (typeStrV == "u32") |
There was a problem hiding this comment.
[minor] here since you already return from if so else if is not needed anymore .
recommendation to have code like follwoing :
if(typeStrV == "i32") {.....; return KvsValue(..);}
if(typeStrV == "u32") {.....; return KvsValue(..);}
if(typeStrV == "i64") {.....; return KvsValue(..);}
.............
return score::MakeUnexpected(ErrorCode::
There was a problem hiding this comment.
I found keeping else if and else in some places to be somewhat more readable, personal preference, fix unnecessary.
Drop single point of return rule.
Functions complexity is decreased by allowing an early return.
Resolves #522