-
Notifications
You must be signed in to change notification settings - Fork 5.9k
Adding Enforce to platform #2646
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
Conversation
dcc7d0c to
09dcc7a
Compare
Basically from caffe2::logging.h, but only expose `PADDLE_ENFORCE` interface.
09dcc7a to
3a119ef
Compare
paddle/platform/enforce.h
Outdated
| @@ -0,0 +1,116 @@ | |||
| /* | |||
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.
The Copyright format is not right, please use this
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.
paddle/platform/enforce_test.cc
Outdated
| @@ -0,0 +1,25 @@ | |||
| #include <gtest/gtest.h> | |||
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.
add Copyright info.
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.
paddle/platform/enforce.h
Outdated
| all_msg_ = sout.str(); | ||
| } | ||
|
|
||
| const char* what() const noexcept override { return all_msg_.c_str(); } |
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 not just return a std::string
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.
because it is the std::exception interface.
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.
got it! thanks~
wangkuiyi
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.
LGTM
paddle/platform/enforce.h
Outdated
|
|
||
| namespace details { | ||
|
|
||
| inline void MakeStringInternal(std::ostringstream& stream) {} |
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 replace MakeString by string::Printf in the near future. I have a PR working on string::Printf.
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, and wait for CI. After passing CI tests, this PR will be merged.
Basically from caffe2::logging.h, but only expose
PADDLE_ENFORCEinterface.