-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
check duplicate of ProtoAndCheckerMaker #2903
check duplicate of ProtoAndCheckerMaker #2903
Conversation
@@ -61,7 +61,15 @@ class OpProtoAndCheckerMaker { | |||
OpProtoAndCheckerMaker(OpProto* proto, OpAttrChecker* op_checker) | |||
: proto_(proto), op_checker_(op_checker) {} | |||
|
|||
~OpProtoAndCheckerMaker() { CheckNoDuplicatedAttrs(); } | |||
~OpProtoAndCheckerMaker() { | |||
PADDLE_ENFORCE(validated_, "should call Validate after build"); |
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 push that validation in the destructor?
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 Exception in destructor can not be caught, so it can not be tested~
paddle/framework/op_registry.h
Outdated
void Validate() { | ||
validated_ = true; | ||
CheckNoDuplicatedAttrs(); | ||
CheckNoDuplicatedInOut(); |
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.
Not only attributes but also input and output together could not be duplicated.
So maybe one function named CheckNoDuplicatedInOutAttrs
is better?
paddle/framework/op_registry.h
Outdated
std::unordered_set<std::string> names; | ||
size_t cnt = 0; | ||
for (auto& input : proto_->inputs()) { | ||
names.insert(input.name()); |
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 use:
for (auto& input : proto_->inputs()) {
std::string input_name = input.name();
PADDLE_ENFORCE(!names.count(input_name), "%s is duplicated!", input_name);
names.insert(input_name);
}
In this way, we are able to be informed about which one is duplicated.
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.
ok, it's a little slower~
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, except one tiny C++ syntax.
paddle/framework/op_registry.h
Outdated
std::unordered_set<std::string> names; | ||
size_t cnt = 0; | ||
auto checker = [&](std::string name) { |
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.
std::string -> const std::string&
remove doubled words
ProtoAndCheckerMaker should not have the same name in attributes or inputs/outputs.