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

Remove OperatorBase::Init #3425

Closed
wangkuiyi opened this issue Aug 11, 2017 · 1 comment · Fixed by #3460
Closed

Remove OperatorBase::Init #3425

wangkuiyi opened this issue Aug 11, 2017 · 1 comment · Fixed by #3460
Assignees

Comments

@wangkuiyi
Copy link
Collaborator

wangkuiyi commented Aug 11, 2017

OperatorBase::Init duplicates with the C++ constructor mechanism. We should remove it.

It seems that people wrote this duplicated mechanism because the constructor of OperatorBase is over-simplified to take no parameters, and OpRegistry::CreateOp would new an operator instance and set OperatorBase:: directly -- the core problem is that OperatorBase's properties are declared public instead of protected.

So, to fix this issue we need to

  1. Make OperatorBase's properties protected

so that we need to

  1. Add property accessors to OpeatorBase Add OperatorBase property accessors #3426
  2. Make OperatorBase and all its sub-classes' constructors able to set property values. Add constructors to OperatorBase and sub-classes #3429
  3. Make grad_op_builder.cc reads from OperatorBase:: via the accessors Make grad_op_bulider.cc use OperatorBase::<property-accessor> #3427
  4. Make op_registry.{h,cc} call new constructor of OperatorBase. Make op_registry.{h,cc} call new constructor of OperatorBase. #3438
  5. Make all writes to OperatorBase:: through constructors.
  6. Remove set OperatorBase properties directly.
  7. Make properties protected. change operator public to protected #3460
@reyoung
Copy link
Collaborator

reyoung commented Aug 23, 2017

Done

@reyoung reyoung closed this as completed Aug 23, 2017
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 a pull request may close this issue.

2 participants