Skip to content
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

Unify Map in OpDescBind #4547

Merged

Conversation

reyoung
Copy link
Collaborator

@reyoung reyoung commented Sep 30, 2017

No description provided.

@@ -134,7 +158,8 @@ void OpDescBind::Sync() {
attr_desc->set_name(attr.first);
attr_desc->set_type(
static_cast<framework::AttrType>(attr.second.which() - 1));
boost::apply_visitor(SetAttrDescVisitor(attr_desc), attr.second);
SetAttrDescVisitor visitor(attr_desc);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is recommended by clang-tidy for code reading.

JiayiFeng
JiayiFeng previously approved these changes Oct 1, 2017
Copy link
Collaborator

@JiayiFeng JiayiFeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@reyoung reyoung merged commit 33c5453 into PaddlePaddle:develop Oct 2, 2017

namespace paddle {
namespace framework {

// The order should be as same as framework.proto
typedef boost::variant<boost::blank, int, float, std::string, std::vector<int>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason of moving type Attribute from attribute.h into a new header files type_defs.h? This PR claims to resolve a cyclic dependency issue, but there is no description of this issue.

@wangkuiyi wangkuiyi mentioned this pull request Oct 4, 2017
@reyoung reyoung deleted the feature/change_bind_data_types branch October 12, 2017 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants