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

[REFACTOR][Relay] Migrate Id ObjectRef to not-null #5748

Closed
wants to merge 1 commit into from

Conversation

ANSHUMAN87
Copy link
Contributor

Refer #5318

@tqchen, @jroesch, @zhiics, @junrushao1994 : Please help review, Thanks!

@@ -152,6 +152,9 @@ class Var;
/*! \brief Container for Var */
class VarNode : public ExprNode {
public:
VarNode(Id vid, Type type_annotation) : vid(vid), type_annotation(type_annotation) {}
VarNode() : vid("") {}
Copy link
Member

Choose a reason for hiding this comment

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

The not-null that involves composite objects are more complicated and this is not necessarily the best approach, let us wait a bit before we find a better solution

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The not-null that involves composite objects are more complicated and this is not necessarily the best approach, let us wait a bit before we find a better solution

@tqchen : Thanks! I also felt that this is not best way. We might have to change Node registration framework i believe. Will wait for your future guidance!

@ANSHUMAN87
Copy link
Contributor Author

This would be a baseline reference for other Relay nodes.
I have one query here. As you see i have to add 1 empty/default constructor here to resolve compile error i faced in TVM_REGISTER_NODE_TYPE(VarNode).

Please suggest the best way to cover this! Thanks!

@junrushao
Copy link
Member

@tqchen Seems that some changes in my WIP PR have affected this PR https://ci.tvm.ai/blue/organizations/jenkins/tvm/detail/PR-5748/1/pipeline. We probably should "make clean" before build wasm

@tqchen
Copy link
Member

tqchen commented Jun 9, 2020

@junrushao1994 good pt, can you send another PR?

@junrushao
Copy link
Member

Do we have conclusions yet on what the best way is to support not-null with composite objects?

@tqchen
Copy link
Member

tqchen commented Jun 10, 2020

@junrushao1994 I will try to send a proposal around UnsafeNull

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants