Feature/clone op #3522
Merged
Feature/clone op #3522
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Clone method will create a new object instance, which is as same as itself. * This is the first step to remove shared_ptr for OperatorBase
Superjomn
reviewed
Aug 16, 2017
paddle/framework/operator.h
Outdated
| // You can also use | ||
| // using PARENT_CLASS::PARENT_CLASS; | ||
| // to use parent's constructor. | ||
| #define DEFINE_OP_CTOR(CLS, PARENT_CLS) \ |
Contributor
There was a problem hiding this comment.
DEFINE_OP_CREATOR 还是 DEFINE_OP_CONSTRUCTOR 的简写？
Collaborator
Author
There was a problem hiding this comment.
Done. Change it to DEFINE_OP_CONSTRUCTOR
| op->Run(scope, cpu_device_context); | ||
| } | ||
|
|
||
| class OperatorClone : public paddle::framework::OperatorBase { |
Contributor
There was a problem hiding this comment.
这个是当成一个普通 op ，train 跑起来的时候去run吗？ 感觉没必要直接写就行了吧
Collaborator
Author
There was a problem hiding this comment.
这是测试一下Clone返回的结果对不对。只是一个单测。
| }; | ||
|
|
||
| class RecurrentGradientOp final : public framework::OperatorBase { | ||
| class RecurrentGradientOp : public framework::OperatorBase { |
Contributor
There was a problem hiding this comment.
这个是 final吧，如果继承，可以继承 RecurrentGradientAlgorithm
Collaborator
Author
There was a problem hiding this comment.
因为注册Op的时候会插入新的Clone函数，这样用户开发Kernel的时候就不需要关注Clone的逻辑了。
所以注册进OpRegistry的Op，不能是Final的。
Make code more clearer
Fit google C++ style
reyoung
commented
Aug 16, 2017
|
|
||
| // Return a new operator instance, which is as same as this. | ||
| // Use unique_ptr to prevent caller forget to delete this pointer. | ||
| virtual std::unique_ptr<OperatorBase> Clone() const = 0; |
Collaborator
Author
There was a problem hiding this comment.
https://google.github.io/styleguide/cppguide.html#Ownership_and_Smart_Pointers
In google C++ style, says
Prefer to use std::unique_ptr to make ownership transfer explicit.
JiayiFeng
reviewed
Aug 17, 2017
| std::string type_; | ||
| // NOTE: in case of OpGrad, inputs_ contains: | ||
| // I (Inputs) | ||
| // I (Inputs)opear |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #3521