chore: Improve the code style by introducing macros #2629
Conversation
AntiTopQuark
force-pushed
the
return_if_error
branch
from
49d821f
to
956a9fd
Compare
|
You can refer to my comment #2521 (comment). |
src/common/status.h
Outdated
| #define RETURN_IF_ERR_FROM_ROCKSDB(expr, error_code, format_str, with_status_string) \ | ||
| ({ \ | ||
| auto&& status = (expr); \ | ||
| if (!status.ok()) { \ | ||
| if (with_status_string) { \ | ||
| return {error_code, fmt::format(format_str, status.ToString())}; \ | ||
| } else { \ | ||
| return {error_code, fmt::format(format_str)}; \ | ||
| } \ | ||
| } \ | ||
| }) |
There was a problem hiding this comment.
Several points:
- Many code of this macro can be implemented by a ctor
Status(rocksdb::Status&&), we don't need write them in the macro; - After such ctor is impl, this macro is useless since it also can be handled by
GET_OR_RET; - This macro doesn't explicit specify the output type, which can trigger some weird issues (the return type can actually be not a
Status, butrocksdb::Status, or evenpair<int, string>).
|
You can check #2630 about how to implement |
AntiTopQuark
force-pushed
the
return_if_error
branch
from
b99cc74
to
2455a1c
Compare
| @@ -397,3 +397,16 @@ bool StatusIsOK(const T& v) { | |||
| if (!StatusIsOK(status)) return std::forward<decltype(status)>(status); \ | |||
| std::forward<decltype(status)>(status); \ | |||
| })) | |||
|
|
|||
| // NOLINTNEXTLINE | |||
| #define RETURN_IF_ERROR(...) \ | |||
There was a problem hiding this comment.
Hmmm I don't think you've got my point. As I've said, this macro is useless and can be replaced by GET_OR_RET. Actually it's function is a subset of GET_OR_RET.
There was a problem hiding this comment.
Hmmm I don't think you've got my point. As I've said, this macro is useless and can be replaced by GET_OR_RET. Actually it's function is a subset of GET_OR_RET.
Firstly, Thank you very much for reviewing my submitted PR and providing valuable feedback! I truly appreciate your input and am grateful for the time you’ve spent helping me improve the code.
This macro can indeed be replaced by GET_OR_RET.
| }) | ||
|
|
||
| // NOLINTNEXTLINE | ||
| #define RETURN_IF_ERR_FROM_ROCKSDB(s, code, msg) \ |
There was a problem hiding this comment.
As I already said, we only need a function/method/ctor/operator T, instead of a macro. If we can complete some task via function/method, please never introduce a new macro.
There was a problem hiding this comment.
As I already said, we only need a function/method/ctor/operator T, instead of a macro. If we can complete some task via function/method, please never introduce a new macro.
Regarding the comment, after careful consideration, I realize that I may not have fully grasped your intention. Specifically, I feel that without introducing a new macro, it becomes challenging to directly return a new Status object.
|
I apologize, but upon reflection, I believe this PR offers minimal value. Additionally, due to my current busy schedule and limited energy, I am unable to continue this work. I apologize once again. cc @PragmaTwice |
Relate Issue: #2521
Introduced two macros for handling error codes:
RETURN_IF_ERRORandRETURN_IF_ERR_FROM_ROCKSDB.I haven't modified all the code yet, so could the project committer please review this code first to see if it’s acceptable?
If approved, I'll go through and update the other files as well.