You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by GitBox <gi...@apache.org> on 2020/02/21 10:02:25 UTC
[GitHub] [incubator-doris] lingbin opened a new pull request #2969: Improve
implementation of Status
lingbin opened a new pull request #2969: Improve implementation of Status
URL: https://github.com/apache/incubator-doris/pull/2969
This patch mainly do:
1. Fixed the implementation of its move-assigment, the `_state` of
the current `Status` object(lhs) should be released;
2. Rename `precise_code` to `posix_code`, a more accurate name;
3. Modify the default value of `posix_code` from `1` to `-1`,
`-1` is a more general errno, and `1` is only used to refer
to `EPERM`(Operation not permitted).
4. Removed unused macros: `RETURN_IF_STATUS_ERROR`
5. Adjusted the order of function declarations, using the same
order as the type of `TStatusCode`, which is more clear.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] imay commented on issue #2969: Improve
implementation of Status
Posted by GitBox <gi...@apache.org>.
imay commented on issue #2969: Improve implementation of Status
URL: https://github.com/apache/incubator-doris/pull/2969#issuecomment-589659754
> My previous understanding was: There is only **one level** of error code defined by Doris, which is `TStatusCode`. According to you, it looks like it will be divided into **two levels** in the future, but is it a bit too much?
The main purpose is to help users determine the problem through error codes. TstatusCode is mainly used to classify the errors that occur, while precise_code is used to determine the specific error
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] lingbin commented on issue #2969: Improve
implementation of Status
Posted by GitBox <gi...@apache.org>.
lingbin commented on issue #2969: Improve implementation of Status
URL: https://github.com/apache/incubator-doris/pull/2969#issuecomment-589654754
> > Is It actually an POSIX errno?
> > For example: For Status::IOError, it is error code is TStatusCode::IO_ERROR, but its "posix_code" may be `EIO`, `ENOTDIR`, `EEXIST` and so on.
>
> Actually no. This field is intended to unify Doris' error code. When user can get a code, he/she will know the specific error. If this is used as posix_code, its usage will be narrowed.
My previous understanding was: There is only **one level** of error code defined by Doris, which is `TStatusCode`. According to you, it looks like it will be divided into **two levels** in the future, but is it a bit too much?
Anyway, I will change it back currently.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] lingbin commented on a change in pull request
#2969: Fixed the move-assigment of `Status`
Posted by GitBox <gi...@apache.org>.
lingbin commented on a change in pull request #2969: Fixed the move-assigment of `Status`
URL: https://github.com/apache/incubator-doris/pull/2969#discussion_r382889942
##########
File path: be/src/common/status.h
##########
@@ -17,114 +17,136 @@ namespace doris {
class Status {
public:
- Status(): _state(nullptr) {}
+ Status() : _state(nullptr) { }
~Status() noexcept { delete[] _state; }
- // copy c'tor makes copy of error detail so Status can be returned by value
- Status(const Status& s)
- : _state(s._state == nullptr ? nullptr : copy_state(s._state)) {
- }
+ // Here _state == nullptr(Status::OK) is the most common case, so use this as
+ // a fast path. This check can avoid one function call in most cases, because
+ // _copy_state() is not an inline function.
+ Status(const Status& s) : _state(s._state == nullptr ? nullptr : _copy_state(s._state)) { }
// same as copy c'tor
Status& operator=(const Status& s) {
// The following condition catches both aliasing (when this == &s),
// and the common case where both s and *this are OK.
if (_state != s._state) {
delete[] _state;
- _state = (s._state == nullptr) ? nullptr : copy_state(s._state);
+ _state = (s._state == nullptr) ? nullptr : _copy_state(s._state);
}
return *this;
}
- // move c'tor
Status(Status&& s) noexcept : _state(s._state) {
s._state = nullptr;
}
- // move assign
Status& operator=(Status&& s) noexcept {
- std::swap(_state, s._state);
+ if (_state != s._state) {
Review comment:
There are no requirements and also no prohibitions in the standard.
But for `dest = std::move(src)`, the following point is determined:
The `move-assignment` of `move-constructor` should have the **same behavior**. Unfortunately, In Doris current implementation, the behavior of the two methods is inconsistent.
For `dest(std::move(src))`, The `move-constructor` of `Status` will clear the state of `src`。
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] lingbin commented on a change in pull request
#2969: Improve implementation of Status
Posted by GitBox <gi...@apache.org>.
lingbin commented on a change in pull request #2969: Improve implementation of Status
URL: https://github.com/apache/incubator-doris/pull/2969#discussion_r382663884
##########
File path: be/src/common/status.cpp
##########
@@ -50,18 +51,20 @@ Status::Status(const PStatus& s) : _state(nullptr) {
TStatusCode::type code = (TStatusCode::type)s.status_code();
if (code != TStatusCode::OK) {
if (s.error_msgs_size() == 0) {
- _state = assemble_state(code, Slice(), 1, Slice());
+ _state = assemble_state(code, Slice(), -1, Slice());
Review comment:
Will precise_code often use `POSIX errno` to indicate detailed error codes in the future?
If it is, what we need to pay attention to is: In POSIX code, `1` is only used to refer
to `EPERM`(`Operation not permitted`), and **`-1` is more general**.
In addition, the type of this precise_code is `int16_t`, a signed integer.
And what is the "serialization problem"?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] lingbin commented on issue #2969: Improve
implementation of Status
Posted by GitBox <gi...@apache.org>.
lingbin commented on issue #2969: Improve implementation of Status
URL: https://github.com/apache/incubator-doris/pull/2969#issuecomment-589649037
> why change precis_code to a posix code?
Is It actually an POSIX errno?
For example: For Status::IOError, it is error code is TStatusCode::IO_ERROR, but its "posix_code" may be `EIO`, `ENOTDIR`, `EEXIST` and so on.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] imay commented on a change in pull request #2969:
Improve implementation of Status
Posted by GitBox <gi...@apache.org>.
imay commented on a change in pull request #2969: Improve implementation of Status
URL: https://github.com/apache/incubator-doris/pull/2969#discussion_r382882975
##########
File path: be/src/common/status.h
##########
@@ -17,114 +17,136 @@ namespace doris {
class Status {
public:
- Status(): _state(nullptr) {}
+ Status() : _state(nullptr) { }
~Status() noexcept { delete[] _state; }
- // copy c'tor makes copy of error detail so Status can be returned by value
- Status(const Status& s)
- : _state(s._state == nullptr ? nullptr : copy_state(s._state)) {
- }
+ // Here _state == nullptr(Status::OK) is the most common case, so use this as
+ // a fast path. This check can avoid one function call in most cases, because
+ // _copy_state() is not an inline function.
+ Status(const Status& s) : _state(s._state == nullptr ? nullptr : _copy_state(s._state)) { }
// same as copy c'tor
Status& operator=(const Status& s) {
// The following condition catches both aliasing (when this == &s),
// and the common case where both s and *this are OK.
if (_state != s._state) {
delete[] _state;
- _state = (s._state == nullptr) ? nullptr : copy_state(s._state);
+ _state = (s._state == nullptr) ? nullptr : _copy_state(s._state);
}
return *this;
}
- // move c'tor
Status(Status&& s) noexcept : _state(s._state) {
s._state = nullptr;
}
- // move assign
Status& operator=(Status&& s) noexcept {
- std::swap(_state, s._state);
+ if (_state != s._state) {
Review comment:
For your example, is it OK to use an object after it has been moved?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] lingbin commented on a change in pull request
#2969: Improve implementation of Status
Posted by GitBox <gi...@apache.org>.
lingbin commented on a change in pull request #2969: Improve implementation of Status
URL: https://github.com/apache/incubator-doris/pull/2969#discussion_r382657616
##########
File path: be/src/common/status.h
##########
@@ -248,49 +256,42 @@ class Status {
};
// some generally useful macros
-#define RETURN_IF_ERROR(stmt) \
- do { \
+#define RETURN_IF_ERROR(stmt) \
+ do { \
const Status& _status_ = (stmt); \
- if (UNLIKELY(!_status_.ok())) { \
- return _status_; \
- } \
- } while (false)
-
-#define RETURN_IF_STATUS_ERROR(status, stmt) \
- do { \
- status = (stmt); \
- if (UNLIKELY(!status.ok())) { \
- return; \
- } \
+ if (UNLIKELY(!_status_.ok())) { \
+ return _status_; \
+ } \
} while (false)
-#define EXIT_IF_ERROR(stmt) \
- do { \
- const Status& _status_ = (stmt); \
- if (UNLIKELY(!_status_.ok())) { \
- string msg = _status_.get_error_msg(); \
- LOG(ERROR) << msg; \
- exit(1); \
- } \
+#define EXIT_IF_ERROR(stmt) \
Review comment:
This macro is basically unchanged, so there is no maintenance issue.
Make `\` on the same column on the right, the code will be clearer, because readers **only need to focus on the implementation on the left** (same as the function). This macro is relatively small, and the advantages are not obvious.
On the other hand, putting `\` in the same column, in addition to being easy to read, also has another advantage, especially the macros that often change, **that it is easy to find errors**, because someone may forget to write `\`. In extreme cases, sometimes compile still will succeed even missing `\`.
Of course, as you said, there is no free lunch. The only problem with this alignment is that the number of diff lines may increase when the code is changed. But I think this "readability" and "easy to find errors" are more important. :-)
Anyway, it is a style problem, if you don't agree, I can change it back.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] lingbin commented on a change in pull request
#2969: Improve implementation of Status
Posted by GitBox <gi...@apache.org>.
lingbin commented on a change in pull request #2969: Improve implementation of Status
URL: https://github.com/apache/incubator-doris/pull/2969#discussion_r382666077
##########
File path: be/src/common/status.h
##########
@@ -17,114 +17,136 @@ namespace doris {
class Status {
public:
- Status(): _state(nullptr) {}
+ Status() : _state(nullptr) { }
~Status() noexcept { delete[] _state; }
- // copy c'tor makes copy of error detail so Status can be returned by value
- Status(const Status& s)
- : _state(s._state == nullptr ? nullptr : copy_state(s._state)) {
- }
+ // Here _state == nullptr(Status::OK) is the most common case, so use this as
+ // a fast path. This check can avoid one function call in most cases, because
+ // _copy_state() is not an inline function.
+ Status(const Status& s) : _state(s._state == nullptr ? nullptr : _copy_state(s._state)) { }
// same as copy c'tor
Status& operator=(const Status& s) {
// The following condition catches both aliasing (when this == &s),
// and the common case where both s and *this are OK.
if (_state != s._state) {
delete[] _state;
- _state = (s._state == nullptr) ? nullptr : copy_state(s._state);
+ _state = (s._state == nullptr) ? nullptr : _copy_state(s._state);
}
return *this;
}
- // move c'tor
Status(Status&& s) noexcept : _state(s._state) {
s._state = nullptr;
}
- // move assign
Status& operator=(Status&& s) noexcept {
- std::swap(_state, s._state);
+ if (_state != s._state) {
Review comment:
> In general, a move assignment operator should:
> 1. Destroy visible resources (though maybe save implementation detail resources).
> 2. Move assign all bases and members.
> 3. If the move assignment of bases and members didn't make the rhs resource-less, then make it so.
Taken from [https://stackoverflow.com/questions/6687388/why-do-some-people-use-swap-for-move-assignments](https://stackoverflow.com/questions/6687388/why-do-some-people-use-swap-for-move-assignments)
A simpler example maybe: somebody may still use the `Status` object after `std::move` it.
For example:
```
Status src = Status::OK();
Status dest = Status::RuntimeError();
dest = std::move(src);
// here, if we use `std::move` in `Status::operator=()`, the `dest` will be
// Status::RuntimeError(), not an empty state(`Status::OK`), which is bug-prune.
if (src.ok())
...
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] lingbin commented on a change in pull request
#2969: Improve implementation of Status
Posted by GitBox <gi...@apache.org>.
lingbin commented on a change in pull request #2969: Improve implementation of Status
URL: https://github.com/apache/incubator-doris/pull/2969#discussion_r382884834
##########
File path: be/src/common/status.h
##########
@@ -248,49 +256,42 @@ class Status {
};
// some generally useful macros
-#define RETURN_IF_ERROR(stmt) \
- do { \
+#define RETURN_IF_ERROR(stmt) \
+ do { \
const Status& _status_ = (stmt); \
- if (UNLIKELY(!_status_.ok())) { \
- return _status_; \
- } \
- } while (false)
-
-#define RETURN_IF_STATUS_ERROR(status, stmt) \
- do { \
- status = (stmt); \
- if (UNLIKELY(!status.ok())) { \
- return; \
- } \
+ if (UNLIKELY(!_status_.ok())) { \
+ return _status_; \
+ } \
} while (false)
-#define EXIT_IF_ERROR(stmt) \
- do { \
- const Status& _status_ = (stmt); \
- if (UNLIKELY(!_status_.ok())) { \
- string msg = _status_.get_error_msg(); \
- LOG(ERROR) << msg; \
- exit(1); \
- } \
+#define EXIT_IF_ERROR(stmt) \
Review comment:
OK, change it back
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] lingbin edited a comment on issue #2969: Improve
implementation of Status
Posted by GitBox <gi...@apache.org>.
lingbin edited a comment on issue #2969: Improve implementation of Status
URL: https://github.com/apache/incubator-doris/pull/2969#issuecomment-589649037
> why change precis_code to a posix code?
Is It actually an POSIX errno?
For example: For Status::IOError, it is error code is `TStatusCode::IO_ERROR`, but its "posix_code" may be `EIO`, `ENOTDIR`, `EEXIST` and so on.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] lingbin commented on a change in pull request
#2969: Fixed the move-assigment of `Status`
Posted by GitBox <gi...@apache.org>.
lingbin commented on a change in pull request #2969: Fixed the move-assigment of `Status`
URL: https://github.com/apache/incubator-doris/pull/2969#discussion_r382889942
##########
File path: be/src/common/status.h
##########
@@ -17,114 +17,136 @@ namespace doris {
class Status {
public:
- Status(): _state(nullptr) {}
+ Status() : _state(nullptr) { }
~Status() noexcept { delete[] _state; }
- // copy c'tor makes copy of error detail so Status can be returned by value
- Status(const Status& s)
- : _state(s._state == nullptr ? nullptr : copy_state(s._state)) {
- }
+ // Here _state == nullptr(Status::OK) is the most common case, so use this as
+ // a fast path. This check can avoid one function call in most cases, because
+ // _copy_state() is not an inline function.
+ Status(const Status& s) : _state(s._state == nullptr ? nullptr : _copy_state(s._state)) { }
// same as copy c'tor
Status& operator=(const Status& s) {
// The following condition catches both aliasing (when this == &s),
// and the common case where both s and *this are OK.
if (_state != s._state) {
delete[] _state;
- _state = (s._state == nullptr) ? nullptr : copy_state(s._state);
+ _state = (s._state == nullptr) ? nullptr : _copy_state(s._state);
}
return *this;
}
- // move c'tor
Status(Status&& s) noexcept : _state(s._state) {
s._state = nullptr;
}
- // move assign
Status& operator=(Status&& s) noexcept {
- std::swap(_state, s._state);
+ if (_state != s._state) {
Review comment:
No, I do not think the standard required that. But for `dest = std::move(src)`, the following two points are determined:
1. the `dest` resource should **not** be assigned to `src` after move-assignment.
2. The move-assignment of move-constructor should have the **same behavior**.
And for `Status`, it only has one member, `_state`, which is **pointer type**.
when the `src::_state` was moved to `dest`, it must be cleared in `src`, otherwise, the pointer will be referenced by both the `src` and `dest`, which will be wrong.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] imay commented on a change in pull request #2969:
Improve implementation of Status
Posted by GitBox <gi...@apache.org>.
imay commented on a change in pull request #2969: Improve implementation of Status
URL: https://github.com/apache/incubator-doris/pull/2969#discussion_r382638860
##########
File path: be/src/common/status.h
##########
@@ -248,49 +256,42 @@ class Status {
};
// some generally useful macros
-#define RETURN_IF_ERROR(stmt) \
- do { \
+#define RETURN_IF_ERROR(stmt) \
+ do { \
const Status& _status_ = (stmt); \
- if (UNLIKELY(!_status_.ok())) { \
- return _status_; \
- } \
- } while (false)
-
-#define RETURN_IF_STATUS_ERROR(status, stmt) \
- do { \
- status = (stmt); \
- if (UNLIKELY(!status.ok())) { \
- return; \
- } \
+ if (UNLIKELY(!_status_.ok())) { \
+ return _status_; \
+ } \
} while (false)
-#define EXIT_IF_ERROR(stmt) \
- do { \
- const Status& _status_ = (stmt); \
- if (UNLIKELY(!_status_.ok())) { \
- string msg = _status_.get_error_msg(); \
- LOG(ERROR) << msg; \
- exit(1); \
- } \
+#define EXIT_IF_ERROR(stmt) \
Review comment:
Emmm, why do this alignment? This will make it more difficult to maintain this
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] imay commented on a change in pull request #2969:
Improve implementation of Status
Posted by GitBox <gi...@apache.org>.
imay commented on a change in pull request #2969: Improve implementation of Status
URL: https://github.com/apache/incubator-doris/pull/2969#discussion_r382882888
##########
File path: be/src/common/status.cpp
##########
@@ -50,18 +51,20 @@ Status::Status(const PStatus& s) : _state(nullptr) {
TStatusCode::type code = (TStatusCode::type)s.status_code();
if (code != TStatusCode::OK) {
if (s.error_msgs_size() == 0) {
- _state = assemble_state(code, Slice(), 1, Slice());
+ _state = assemble_state(code, Slice(), -1, Slice());
Review comment:
This has no relationship with posix code. For user reference, positive numbers are more convenient than negative numbers.
When doing serialization, positive number can be encoded into smaller space, for example varint encode.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] lingbin commented on a change in pull request
#2969: Improve implementation of Status
Posted by GitBox <gi...@apache.org>.
lingbin commented on a change in pull request #2969: Improve implementation of Status
URL: https://github.com/apache/incubator-doris/pull/2969#discussion_r382666077
##########
File path: be/src/common/status.h
##########
@@ -17,114 +17,136 @@ namespace doris {
class Status {
public:
- Status(): _state(nullptr) {}
+ Status() : _state(nullptr) { }
~Status() noexcept { delete[] _state; }
- // copy c'tor makes copy of error detail so Status can be returned by value
- Status(const Status& s)
- : _state(s._state == nullptr ? nullptr : copy_state(s._state)) {
- }
+ // Here _state == nullptr(Status::OK) is the most common case, so use this as
+ // a fast path. This check can avoid one function call in most cases, because
+ // _copy_state() is not an inline function.
+ Status(const Status& s) : _state(s._state == nullptr ? nullptr : _copy_state(s._state)) { }
// same as copy c'tor
Status& operator=(const Status& s) {
// The following condition catches both aliasing (when this == &s),
// and the common case where both s and *this are OK.
if (_state != s._state) {
delete[] _state;
- _state = (s._state == nullptr) ? nullptr : copy_state(s._state);
+ _state = (s._state == nullptr) ? nullptr : _copy_state(s._state);
}
return *this;
}
- // move c'tor
Status(Status&& s) noexcept : _state(s._state) {
s._state = nullptr;
}
- // move assign
Status& operator=(Status&& s) noexcept {
- std::swap(_state, s._state);
+ if (_state != s._state) {
Review comment:
> In general, a move assignment operator should:
> 1. Destroy visible resources (though maybe save implementation detail resources).
> 2. Move assign all bases and members.
> 3. If the move assignment of bases and members didn't make the rhs resource-less, then make it so.
Taken from [https://stackoverflow.com/questions/6687388/why-do-some-people-use-swap-for-move-assignments](https://stackoverflow.com/questions/6687388/why-do-some-people-use-swap-for-move-assignments)
A simpler example maybe: somebody may still use the `Status` object after `std::move` it.
For example:
```
Status src = Status::OK();
Status dest = Status::RuntimeError();
dest = std::move(src);
// here, if we use `std::move` in `Status::operator=()`, the `src` will be
// Status::RuntimeError(), not an empty state(`Status::OK`), which is bug-prune.
if (src.ok())
...
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] imay commented on issue #2969: Improve
implementation of Status
Posted by GitBox <gi...@apache.org>.
imay commented on issue #2969: Improve implementation of Status
URL: https://github.com/apache/incubator-doris/pull/2969#issuecomment-589652338
> Is It actually an POSIX errno?
>
> For example: For Status::IOError, it is error code is TStatusCode::IO_ERROR, but its "posix_code" may be `EIO`, `ENOTDIR`, `EEXIST` and so on.
Actually no. This field is intended to unify Doris' error code. When user can get a code, he/she will know the specific error. If this is used as posix_code, its usage will be narrowed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] imay commented on a change in pull request #2969:
Improve implementation of Status
Posted by GitBox <gi...@apache.org>.
imay commented on a change in pull request #2969: Improve implementation of Status
URL: https://github.com/apache/incubator-doris/pull/2969#discussion_r382882544
##########
File path: be/src/common/status.h
##########
@@ -248,49 +256,42 @@ class Status {
};
// some generally useful macros
-#define RETURN_IF_ERROR(stmt) \
- do { \
+#define RETURN_IF_ERROR(stmt) \
+ do { \
const Status& _status_ = (stmt); \
- if (UNLIKELY(!_status_.ok())) { \
- return _status_; \
- } \
- } while (false)
-
-#define RETURN_IF_STATUS_ERROR(status, stmt) \
- do { \
- status = (stmt); \
- if (UNLIKELY(!status.ok())) { \
- return; \
- } \
+ if (UNLIKELY(!_status_.ok())) { \
+ return _status_; \
+ } \
} while (false)
-#define EXIT_IF_ERROR(stmt) \
- do { \
- const Status& _status_ = (stmt); \
- if (UNLIKELY(!_status_.ok())) { \
- string msg = _status_.get_error_msg(); \
- LOG(ERROR) << msg; \
- exit(1); \
- } \
+#define EXIT_IF_ERROR(stmt) \
Review comment:
The maintain overhead is that if some one to change this code, he/she will realign these code. I think this is a matter of personal preference. Try to keep the original code unchanged.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] lingbin commented on a change in pull request
#2969: Fixed the move-assigment of `Status`
Posted by GitBox <gi...@apache.org>.
lingbin commented on a change in pull request #2969: Fixed the move-assigment of `Status`
URL: https://github.com/apache/incubator-doris/pull/2969#discussion_r382889942
##########
File path: be/src/common/status.h
##########
@@ -17,114 +17,136 @@ namespace doris {
class Status {
public:
- Status(): _state(nullptr) {}
+ Status() : _state(nullptr) { }
~Status() noexcept { delete[] _state; }
- // copy c'tor makes copy of error detail so Status can be returned by value
- Status(const Status& s)
- : _state(s._state == nullptr ? nullptr : copy_state(s._state)) {
- }
+ // Here _state == nullptr(Status::OK) is the most common case, so use this as
+ // a fast path. This check can avoid one function call in most cases, because
+ // _copy_state() is not an inline function.
+ Status(const Status& s) : _state(s._state == nullptr ? nullptr : _copy_state(s._state)) { }
// same as copy c'tor
Status& operator=(const Status& s) {
// The following condition catches both aliasing (when this == &s),
// and the common case where both s and *this are OK.
if (_state != s._state) {
delete[] _state;
- _state = (s._state == nullptr) ? nullptr : copy_state(s._state);
+ _state = (s._state == nullptr) ? nullptr : _copy_state(s._state);
}
return *this;
}
- // move c'tor
Status(Status&& s) noexcept : _state(s._state) {
s._state = nullptr;
}
- // move assign
Status& operator=(Status&& s) noexcept {
- std::swap(_state, s._state);
+ if (_state != s._state) {
Review comment:
No, I do not think the standard required that. But for `dest = std::move(src)`, the following two points are determined:
1. the `dest` resource should **not** be assigned to `src` after move-assignment.
2. The move-assignment of move-constructor should have the **same behavior**.
And for `Status`, it only has one member, `_state`, which is **pointer type**.
when the `src::_state` was moved to `dest`, it must be cleared in `src`, otherwise, **the pointer will be referenced by both the `src` and `dest`, which will be wrong.**
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] imay commented on a change in pull request #2969:
Improve implementation of Status
Posted by GitBox <gi...@apache.org>.
imay commented on a change in pull request #2969: Improve implementation of Status
URL: https://github.com/apache/incubator-doris/pull/2969#discussion_r382640877
##########
File path: be/src/common/status.h
##########
@@ -17,114 +17,136 @@ namespace doris {
class Status {
public:
- Status(): _state(nullptr) {}
+ Status() : _state(nullptr) { }
~Status() noexcept { delete[] _state; }
- // copy c'tor makes copy of error detail so Status can be returned by value
- Status(const Status& s)
- : _state(s._state == nullptr ? nullptr : copy_state(s._state)) {
- }
+ // Here _state == nullptr(Status::OK) is the most common case, so use this as
+ // a fast path. This check can avoid one function call in most cases, because
+ // _copy_state() is not an inline function.
+ Status(const Status& s) : _state(s._state == nullptr ? nullptr : _copy_state(s._state)) { }
// same as copy c'tor
Status& operator=(const Status& s) {
// The following condition catches both aliasing (when this == &s),
// and the common case where both s and *this are OK.
if (_state != s._state) {
delete[] _state;
- _state = (s._state == nullptr) ? nullptr : copy_state(s._state);
+ _state = (s._state == nullptr) ? nullptr : _copy_state(s._state);
}
return *this;
}
- // move c'tor
Status(Status&& s) noexcept : _state(s._state) {
s._state = nullptr;
}
- // move assign
Status& operator=(Status&& s) noexcept {
- std::swap(_state, s._state);
+ if (_state != s._state) {
Review comment:
what's wrong with std::swap?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] lingbin commented on a change in pull request
#2969: Improve implementation of Status
Posted by GitBox <gi...@apache.org>.
lingbin commented on a change in pull request #2969: Improve implementation of Status
URL: https://github.com/apache/incubator-doris/pull/2969#discussion_r382666077
##########
File path: be/src/common/status.h
##########
@@ -17,114 +17,136 @@ namespace doris {
class Status {
public:
- Status(): _state(nullptr) {}
+ Status() : _state(nullptr) { }
~Status() noexcept { delete[] _state; }
- // copy c'tor makes copy of error detail so Status can be returned by value
- Status(const Status& s)
- : _state(s._state == nullptr ? nullptr : copy_state(s._state)) {
- }
+ // Here _state == nullptr(Status::OK) is the most common case, so use this as
+ // a fast path. This check can avoid one function call in most cases, because
+ // _copy_state() is not an inline function.
+ Status(const Status& s) : _state(s._state == nullptr ? nullptr : _copy_state(s._state)) { }
// same as copy c'tor
Status& operator=(const Status& s) {
// The following condition catches both aliasing (when this == &s),
// and the common case where both s and *this are OK.
if (_state != s._state) {
delete[] _state;
- _state = (s._state == nullptr) ? nullptr : copy_state(s._state);
+ _state = (s._state == nullptr) ? nullptr : _copy_state(s._state);
}
return *this;
}
- // move c'tor
Status(Status&& s) noexcept : _state(s._state) {
s._state = nullptr;
}
- // move assign
Status& operator=(Status&& s) noexcept {
- std::swap(_state, s._state);
+ if (_state != s._state) {
Review comment:
> In general, a move assignment operator should:
> 1. Destroy visible resources (though maybe save implementation detail resources).
> 2. Move assign all bases and members.
> 3. If the move assignment of bases and members didn't make the rhs resource-less, then make it so.
Taken from [https://stackoverflow.com/questions/6687388/why-do-some-people-use-swap-for-move-assignments](https://stackoverflow.com/questions/6687388/why-do-some-people-use-swap-for-move-assignments)
A simpler example maybe: somebody may still use the `Status` object after `std::move` it.
For example:
```
Status src = Status::RuntimeError();
Status dest = Status::OK()
dest = std::move(src);
// here, if we use `std::move` in `Status::operator=()`, the `dest` will be
// Status::RuntimeError(), not an empty state(`Status::OK`), which is bug-prune.
if (src.ok())
...
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] lingbin commented on a change in pull request
#2969: Fixed the move-assigment of `Status`
Posted by GitBox <gi...@apache.org>.
lingbin commented on a change in pull request #2969: Fixed the move-assigment of `Status`
URL: https://github.com/apache/incubator-doris/pull/2969#discussion_r382889942
##########
File path: be/src/common/status.h
##########
@@ -17,114 +17,136 @@ namespace doris {
class Status {
public:
- Status(): _state(nullptr) {}
+ Status() : _state(nullptr) { }
~Status() noexcept { delete[] _state; }
- // copy c'tor makes copy of error detail so Status can be returned by value
- Status(const Status& s)
- : _state(s._state == nullptr ? nullptr : copy_state(s._state)) {
- }
+ // Here _state == nullptr(Status::OK) is the most common case, so use this as
+ // a fast path. This check can avoid one function call in most cases, because
+ // _copy_state() is not an inline function.
+ Status(const Status& s) : _state(s._state == nullptr ? nullptr : _copy_state(s._state)) { }
// same as copy c'tor
Status& operator=(const Status& s) {
// The following condition catches both aliasing (when this == &s),
// and the common case where both s and *this are OK.
if (_state != s._state) {
delete[] _state;
- _state = (s._state == nullptr) ? nullptr : copy_state(s._state);
+ _state = (s._state == nullptr) ? nullptr : _copy_state(s._state);
}
return *this;
}
- // move c'tor
Status(Status&& s) noexcept : _state(s._state) {
s._state = nullptr;
}
- // move assign
Status& operator=(Status&& s) noexcept {
- std::swap(_state, s._state);
+ if (_state != s._state) {
Review comment:
There are no requirements and also no prohibitions in the standard.
But for `dest = std::move(src)`, the following point is determined:
The `move-assignment` of `move-constructor` should have the **same behavior**. Unfortunately, In Doris current implementation, the behavior of the two methods is inconsistent.
And for `Status`, it only has one member, `_state`, which is **pointer type**.
when the `src::_state` was moved to `dest`, it must be cleared in `src`, otherwise, **the pointer will be referenced by both the `src` and `dest`, which will be wrong.**
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] lingbin commented on a change in pull request
#2969: Fixed the move-assigment of `Status`
Posted by GitBox <gi...@apache.org>.
lingbin commented on a change in pull request #2969: Fixed the move-assigment of `Status`
URL: https://github.com/apache/incubator-doris/pull/2969#discussion_r382889942
##########
File path: be/src/common/status.h
##########
@@ -17,114 +17,136 @@ namespace doris {
class Status {
public:
- Status(): _state(nullptr) {}
+ Status() : _state(nullptr) { }
~Status() noexcept { delete[] _state; }
- // copy c'tor makes copy of error detail so Status can be returned by value
- Status(const Status& s)
- : _state(s._state == nullptr ? nullptr : copy_state(s._state)) {
- }
+ // Here _state == nullptr(Status::OK) is the most common case, so use this as
+ // a fast path. This check can avoid one function call in most cases, because
+ // _copy_state() is not an inline function.
+ Status(const Status& s) : _state(s._state == nullptr ? nullptr : _copy_state(s._state)) { }
// same as copy c'tor
Status& operator=(const Status& s) {
// The following condition catches both aliasing (when this == &s),
// and the common case where both s and *this are OK.
if (_state != s._state) {
delete[] _state;
- _state = (s._state == nullptr) ? nullptr : copy_state(s._state);
+ _state = (s._state == nullptr) ? nullptr : _copy_state(s._state);
}
return *this;
}
- // move c'tor
Status(Status&& s) noexcept : _state(s._state) {
s._state = nullptr;
}
- // move assign
Status& operator=(Status&& s) noexcept {
- std::swap(_state, s._state);
+ if (_state != s._state) {
Review comment:
There are no requirements and also no prohibitions in the standard.
But for `dest = std::move(src)`, the following two points are determined:
1. the `dest` resource should **not** be assigned to `src` after move-assignment.
2. The move-assignment of move-constructor should have the **same behavior**.
And for `Status`, it only has one member, `_state`, which is **pointer type**.
when the `src::_state` was moved to `dest`, it must be cleared in `src`, otherwise, **the pointer will be referenced by both the `src` and `dest`, which will be wrong.**
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] imay commented on a change in pull request #2969:
Fixed the move-assigment of `Status`
Posted by GitBox <gi...@apache.org>.
imay commented on a change in pull request #2969: Fixed the move-assigment of `Status`
URL: https://github.com/apache/incubator-doris/pull/2969#discussion_r382889253
##########
File path: be/src/common/status.h
##########
@@ -17,114 +17,136 @@ namespace doris {
class Status {
public:
- Status(): _state(nullptr) {}
+ Status() : _state(nullptr) { }
~Status() noexcept { delete[] _state; }
- // copy c'tor makes copy of error detail so Status can be returned by value
- Status(const Status& s)
- : _state(s._state == nullptr ? nullptr : copy_state(s._state)) {
- }
+ // Here _state == nullptr(Status::OK) is the most common case, so use this as
+ // a fast path. This check can avoid one function call in most cases, because
+ // _copy_state() is not an inline function.
+ Status(const Status& s) : _state(s._state == nullptr ? nullptr : _copy_state(s._state)) { }
// same as copy c'tor
Status& operator=(const Status& s) {
// The following condition catches both aliasing (when this == &s),
// and the common case where both s and *this are OK.
if (_state != s._state) {
delete[] _state;
- _state = (s._state == nullptr) ? nullptr : copy_state(s._state);
+ _state = (s._state == nullptr) ? nullptr : _copy_state(s._state);
}
return *this;
}
- // move c'tor
Status(Status&& s) noexcept : _state(s._state) {
s._state = nullptr;
}
- // move assign
Status& operator=(Status&& s) noexcept {
- std::swap(_state, s._state);
+ if (_state != s._state) {
Review comment:
What I am worried about is that the C ++ standard requires that it must be initialized after the move?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] lingbin commented on a change in pull request
#2969: Fixed the move-assigment of `Status`
Posted by GitBox <gi...@apache.org>.
lingbin commented on a change in pull request #2969: Fixed the move-assigment of `Status`
URL: https://github.com/apache/incubator-doris/pull/2969#discussion_r382889942
##########
File path: be/src/common/status.h
##########
@@ -17,114 +17,136 @@ namespace doris {
class Status {
public:
- Status(): _state(nullptr) {}
+ Status() : _state(nullptr) { }
~Status() noexcept { delete[] _state; }
- // copy c'tor makes copy of error detail so Status can be returned by value
- Status(const Status& s)
- : _state(s._state == nullptr ? nullptr : copy_state(s._state)) {
- }
+ // Here _state == nullptr(Status::OK) is the most common case, so use this as
+ // a fast path. This check can avoid one function call in most cases, because
+ // _copy_state() is not an inline function.
+ Status(const Status& s) : _state(s._state == nullptr ? nullptr : _copy_state(s._state)) { }
// same as copy c'tor
Status& operator=(const Status& s) {
// The following condition catches both aliasing (when this == &s),
// and the common case where both s and *this are OK.
if (_state != s._state) {
delete[] _state;
- _state = (s._state == nullptr) ? nullptr : copy_state(s._state);
+ _state = (s._state == nullptr) ? nullptr : _copy_state(s._state);
}
return *this;
}
- // move c'tor
Status(Status&& s) noexcept : _state(s._state) {
s._state = nullptr;
}
- // move assign
Status& operator=(Status&& s) noexcept {
- std::swap(_state, s._state);
+ if (_state != s._state) {
Review comment:
There are no requirements and also no prohibitions in the standard. i.e., It is undefined.
But for `dest = std::move(src)`, the following two points are determined:
1. the `dest` resource should **not** be assigned to `src` after move-assignment.
2. The move-assignment of move-constructor should have the **same behavior**.
And for `Status`, it only has one member, `_state`, which is **pointer type**.
when the `src::_state` was moved to `dest`, it must be cleared in `src`, otherwise, **the pointer will be referenced by both the `src` and `dest`, which will be wrong.**
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] lingbin commented on a change in pull request
#2969: Improve implementation of Status
Posted by GitBox <gi...@apache.org>.
lingbin commented on a change in pull request #2969: Improve implementation of Status
URL: https://github.com/apache/incubator-doris/pull/2969#discussion_r382666077
##########
File path: be/src/common/status.h
##########
@@ -17,114 +17,136 @@ namespace doris {
class Status {
public:
- Status(): _state(nullptr) {}
+ Status() : _state(nullptr) { }
~Status() noexcept { delete[] _state; }
- // copy c'tor makes copy of error detail so Status can be returned by value
- Status(const Status& s)
- : _state(s._state == nullptr ? nullptr : copy_state(s._state)) {
- }
+ // Here _state == nullptr(Status::OK) is the most common case, so use this as
+ // a fast path. This check can avoid one function call in most cases, because
+ // _copy_state() is not an inline function.
+ Status(const Status& s) : _state(s._state == nullptr ? nullptr : _copy_state(s._state)) { }
// same as copy c'tor
Status& operator=(const Status& s) {
// The following condition catches both aliasing (when this == &s),
// and the common case where both s and *this are OK.
if (_state != s._state) {
delete[] _state;
- _state = (s._state == nullptr) ? nullptr : copy_state(s._state);
+ _state = (s._state == nullptr) ? nullptr : _copy_state(s._state);
}
return *this;
}
- // move c'tor
Status(Status&& s) noexcept : _state(s._state) {
s._state = nullptr;
}
- // move assign
Status& operator=(Status&& s) noexcept {
- std::swap(_state, s._state);
+ if (_state != s._state) {
Review comment:
Taken from [https://stackoverflow.com/questions/6687388/why-do-some-people-use-swap-for-move-assignments](https://stackoverflow.com/questions/6687388/why-do-some-people-use-swap-for-move-assignments)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] lingbin commented on a change in pull request
#2969: Improve implementation of Status
Posted by GitBox <gi...@apache.org>.
lingbin commented on a change in pull request #2969: Improve implementation of Status
URL: https://github.com/apache/incubator-doris/pull/2969#discussion_r382884847
##########
File path: be/src/common/status.cpp
##########
@@ -50,18 +51,20 @@ Status::Status(const PStatus& s) : _state(nullptr) {
TStatusCode::type code = (TStatusCode::type)s.status_code();
if (code != TStatusCode::OK) {
if (s.error_msgs_size() == 0) {
- _state = assemble_state(code, Slice(), 1, Slice());
+ _state = assemble_state(code, Slice(), -1, Slice());
Review comment:
OK, change it to `1`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] imay commented on a change in pull request #2969:
Improve implementation of Status
Posted by GitBox <gi...@apache.org>.
imay commented on a change in pull request #2969: Improve implementation of Status
URL: https://github.com/apache/incubator-doris/pull/2969#discussion_r382639919
##########
File path: be/src/common/status.cpp
##########
@@ -50,18 +51,20 @@ Status::Status(const PStatus& s) : _state(nullptr) {
TStatusCode::type code = (TStatusCode::type)s.status_code();
if (code != TStatusCode::OK) {
if (s.error_msgs_size() == 0) {
- _state = assemble_state(code, Slice(), 1, Slice());
+ _state = assemble_state(code, Slice(), -1, Slice());
Review comment:
keep 1 as default code? Because minus number is not good for serialization.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] lingbin commented on a change in pull request
#2969: Improve implementation of Status
Posted by GitBox <gi...@apache.org>.
lingbin commented on a change in pull request #2969: Improve implementation of Status
URL: https://github.com/apache/incubator-doris/pull/2969#discussion_r382666077
##########
File path: be/src/common/status.h
##########
@@ -17,114 +17,136 @@ namespace doris {
class Status {
public:
- Status(): _state(nullptr) {}
+ Status() : _state(nullptr) { }
~Status() noexcept { delete[] _state; }
- // copy c'tor makes copy of error detail so Status can be returned by value
- Status(const Status& s)
- : _state(s._state == nullptr ? nullptr : copy_state(s._state)) {
- }
+ // Here _state == nullptr(Status::OK) is the most common case, so use this as
+ // a fast path. This check can avoid one function call in most cases, because
+ // _copy_state() is not an inline function.
+ Status(const Status& s) : _state(s._state == nullptr ? nullptr : _copy_state(s._state)) { }
// same as copy c'tor
Status& operator=(const Status& s) {
// The following condition catches both aliasing (when this == &s),
// and the common case where both s and *this are OK.
if (_state != s._state) {
delete[] _state;
- _state = (s._state == nullptr) ? nullptr : copy_state(s._state);
+ _state = (s._state == nullptr) ? nullptr : _copy_state(s._state);
}
return *this;
}
- // move c'tor
Status(Status&& s) noexcept : _state(s._state) {
s._state = nullptr;
}
- // move assign
Status& operator=(Status&& s) noexcept {
- std::swap(_state, s._state);
+ if (_state != s._state) {
Review comment:
> In general, a move assignment operator should:
> 1. Destroy visible resources (though maybe save implementation detail resources).
> 2. Move assign all bases and members.
> 3. If the move assignment of bases and members didn't make the rhs resource-less, then make it so.
Taken from [https://stackoverflow.com/questions/6687388/why-do-some-people-use-swap-for-move-assignments](https://stackoverflow.com/questions/6687388/why-do-some-people-use-swap-for-move-assignments)
Another example is, somebody may still use the the `Status` object after move.
```
Status src = Status::RuntimeError();
Status dest = Status::IOError("xxx")
dest = std::move(src);
// here, if we use `std::move` in `Status::operator=`
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] lingbin commented on a change in pull request
#2969: Improve implementation of Status
Posted by GitBox <gi...@apache.org>.
lingbin commented on a change in pull request #2969: Improve implementation of Status
URL: https://github.com/apache/incubator-doris/pull/2969#discussion_r382885009
##########
File path: be/src/common/status.h
##########
@@ -17,114 +17,136 @@ namespace doris {
class Status {
public:
- Status(): _state(nullptr) {}
+ Status() : _state(nullptr) { }
~Status() noexcept { delete[] _state; }
- // copy c'tor makes copy of error detail so Status can be returned by value
- Status(const Status& s)
- : _state(s._state == nullptr ? nullptr : copy_state(s._state)) {
- }
+ // Here _state == nullptr(Status::OK) is the most common case, so use this as
+ // a fast path. This check can avoid one function call in most cases, because
+ // _copy_state() is not an inline function.
+ Status(const Status& s) : _state(s._state == nullptr ? nullptr : _copy_state(s._state)) { }
// same as copy c'tor
Status& operator=(const Status& s) {
// The following condition catches both aliasing (when this == &s),
// and the common case where both s and *this are OK.
if (_state != s._state) {
delete[] _state;
- _state = (s._state == nullptr) ? nullptr : copy_state(s._state);
+ _state = (s._state == nullptr) ? nullptr : _copy_state(s._state);
}
return *this;
}
- // move c'tor
Status(Status&& s) noexcept : _state(s._state) {
s._state = nullptr;
}
- // move assign
Status& operator=(Status&& s) noexcept {
- std::swap(_state, s._state);
+ if (_state != s._state) {
Review comment:
Yes, it is ok to use a variable after `std::move()` it.
At least the compiler will not prevent anyone from using it this way.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org