-
Notifications
You must be signed in to change notification settings - Fork 5.9k
Feature/moving error to framework #2601
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
Feature/moving error to framework #2601
Conversation
Make paddle framework use Error as Error Type.
typhoonzero
left a comment
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.
Is this PR fix #2430?
|
|
||
| private: | ||
| private: | ||
| std::shared_ptr<std::string> msg_; |
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.
Why a shared_ptr, can ::std::string object do the job?
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.
No. Because if we using std:: string, Error will copy by value each time, which is very time-consuming.
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.
Well, Error object creation will do one memory operation. But after that, we'll pass error object using reference all the time, we can also use Error as return types of functions as we have return value optimization
This is the sample code from this PR:
Error __must_check foobar() {
Error err;
// do something.
foo(&err);
if (err) return err;
}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.
We should not mainly use foo(Error* error) to return Error. __must_check will let developer force to check the return error, but foo(Error* error) does not have the same technic. So it is better to use Error __must_check foo();.
In another hand, RVO depends on compiler implementation.
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'm thinking of two type of use case, which is already in this PR:
// single error return value
Error err = foo();
// multiple return values with Error
Error err;
ReturnValue ret;
foo(&err, &ret);
// more general uses, mostly use error as return value.
ReturnValue ret;
Error err = foo(&ret, "params");
| * removed later. | ||
| */ | ||
| void check() const { CHECK(this->isOK()) << msg(); } | ||
| bool operator==(const Error &o) const { |
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.
@typhoonzero By using msg_ a shared_ptr, the operator== will be fast for predefined Error.
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 see.
paddle/framework/error.h
Outdated
| * @brief isOK return True if there is no error. | ||
| * @return True if no error. | ||
| */ | ||
| bool isOK() const { return msg_ == nullptr; } |
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 am not very familiar with C++ naming convention so I could be wrong: to me it seems ok() is better than isOK() (btw, maybe should be isOk rather than isOK according to Google C++ style?), "is" makes developers type two more characters, and does not provide much value in my opinion.
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.
My fault, It will be changed soon.
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.
Done.
|
Do we need error code to specify what kind of error it is? Two errors with different error message could be the same kind of error (e.g., |
In this case, maybe a global function bool IsNotFound(const Error& err) {
auto pattern = r"file not found: .*";
return pattern.math(err.msg());
}
And I think the most error of Paddle should not be a dynamic string. The most error should be just a predefined const string. const Error FileNotFound = Error("file is not found.");Then |
following google name style.
|
Closed because #2646 |
Move
paddle/utils/Error.htopaddle/framework/error.h.Make new framework using
paddle::Erroras error type.