You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2021/12/15 08:59:48 UTC
[GitHub] [tvm] yzh119 opened a new pull request #9748: Overload get() function for `Optional` type.
yzh119 opened a new pull request #9748:
URL: https://github.com/apache/tvm/pull/9748
Currently `Optional` don't have its own `get()` implementation, so if we want to get the internal container pointer of an optional object `o` with type `Optional<T>`, we can either use `o.value().get()` (in the case it's defined) or `static_cast<const T*>(o.get())` (because the inherited `get()` function has no type information). Neither of them are satisfactory if we want to get a pointer with type `const T*`.
This PR overloads the `get()` function of `Optional`, which returns `nullptr` when the object is not defined, and it's corresponding data pointer otherwise.
cc @junrushao1994 @tqchen
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #9748: Overload get() function for `Optional` type.
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #9748:
URL: https://github.com/apache/tvm/pull/9748#issuecomment-1035913080
@yzh119 Would you like to rebase and retrigger the CI? Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on a change in pull request #9748: Overload get() function for `Optional` type.
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #9748:
URL: https://github.com/apache/tvm/pull/9748#discussion_r807654206
##########
File path: include/tvm/runtime/container/optional.h
##########
@@ -93,6 +93,11 @@ class Optional : public ObjectRef {
ICHECK(data_ != nullptr);
return T(data_);
}
+ /*!
+ * \return The internal object pointer with container type of T.
+ * \note This function do not perform not-null checking.
+ */
+ const ContainerType* get() const { return static_cast<ContainerType*>(data_.get()); }
Review comment:
Thanks @tqchen for the valuable feedback! On the other hand, I would say this PR actually does is to simply improve the `Optional::get` method to return the underlying type instead of the plain Object pointer, and hence it might make sense to keep the name as it is. What do you think>?
##########
File path: include/tvm/runtime/container/optional.h
##########
@@ -93,6 +93,11 @@ class Optional : public ObjectRef {
ICHECK(data_ != nullptr);
return T(data_);
}
+ /*!
+ * \return The internal object pointer with container type of T.
+ * \note This function do not perform not-null checking.
+ */
+ const ContainerType* get() const { return static_cast<ContainerType*>(data_.get()); }
Review comment:
Thanks @tqchen for the valuable feedback! On the other hand, I would say this PR actually does is to simply improve the `Optional::get` method to return the underlying type instead of the plain Object pointer, and hence it might make sense to keep the name as it is. What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #9748: Overload get() function for `Optional` type.
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #9748:
URL: https://github.com/apache/tvm/pull/9748#issuecomment-1042340643
Thanks @tqchen @yzh119!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] yzh119 commented on pull request #9748: Overload get() function for `Optional` type.
Posted by GitBox <gi...@apache.org>.
yzh119 commented on pull request #9748:
URL: https://github.com/apache/tvm/pull/9748#issuecomment-994640535
CI somehow failed, I'm trying to figure it out.
On Wed, Dec 15, 2021, 1:49 AM Junru Shao ***@***.***> wrote:
> Why close this PR? I'm very interested
>
> —
> You are receiving this because you modified the open/close state.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/tvm/pull/9748#issuecomment-994588698>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ACZ2NM3O2FN62RIBIOPSPV3URBQELANCNFSM5KDBWGQA>
> .
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on a change in pull request #9748: Overload get() function for `Optional` type.
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #9748:
URL: https://github.com/apache/tvm/pull/9748#discussion_r805954346
##########
File path: include/tvm/runtime/container/optional.h
##########
@@ -93,6 +93,11 @@ class Optional : public ObjectRef {
ICHECK(data_ != nullptr);
return T(data_);
}
+ /*!
+ * \return The internal object pointer with container type of T.
+ * \note This function do not perform not-null checking.
+ */
+ const ContainerType* get() const { return static_cast<ContainerType*>(data_.get()); }
Review comment:
Note that get is not a standard API as per stl style for Optional. While `get` is a standard stl style name. We might want to have a longer name in this case.
How about `GetContainerPtr`(use CamelCase to indicate that it is not STL compatible).
The comment can be updated to explain the behavior of nullopt case(return nullptr) and non-nullopt case(returns the container ptr)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] yzh119 commented on pull request #9748: Overload get() function for `Optional` type.
Posted by GitBox <gi...@apache.org>.
yzh119 commented on pull request #9748:
URL: https://github.com/apache/tvm/pull/9748#issuecomment-1017052806
I think the CI was broken because of some random numerical issue and I'll re-trigger it, @junrushao1994 any further comments on this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #9748: Overload get() function for `Optional` type.
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #9748:
URL: https://github.com/apache/tvm/pull/9748#issuecomment-1017154042
Yes, it's a bit of flaky. Let me retrigger this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on a change in pull request #9748: Overload get() function for `Optional` type.
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #9748:
URL: https://github.com/apache/tvm/pull/9748#discussion_r805954346
##########
File path: include/tvm/runtime/container/optional.h
##########
@@ -93,6 +93,11 @@ class Optional : public ObjectRef {
ICHECK(data_ != nullptr);
return T(data_);
}
+ /*!
+ * \return The internal object pointer with container type of T.
+ * \note This function do not perform not-null checking.
+ */
+ const ContainerType* get() const { return static_cast<ContainerType*>(data_.get()); }
Review comment:
Note that get is not a standard API as per stl style for Optional. While `get` is a standard stl style name. We might want to have a longer name in this case.
How about `GetContainerPtr`(use CamelCase to indicate that it is not STL compatible).
The comment can be updated to explain the behavior of nullopt case(return nullptr) and non-nullopt case(returns the container ptr)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] yzh119 commented on pull request #9748: Overload get() function for `Optional` type.
Posted by GitBox <gi...@apache.org>.
yzh119 commented on pull request #9748:
URL: https://github.com/apache/tvm/pull/9748#issuecomment-1035915377
@junrushao1994 done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] yzh119 closed pull request #9748: Overload get() function for `Optional` type.
Posted by GitBox <gi...@apache.org>.
yzh119 closed pull request #9748:
URL: https://github.com/apache/tvm/pull/9748
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #9748: Overload get() function for `Optional` type.
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #9748:
URL: https://github.com/apache/tvm/pull/9748#issuecomment-994588698
Why close this PR? I'm very interested
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] yzh119 commented on a change in pull request #9748: Overload get() function for `Optional` type.
Posted by GitBox <gi...@apache.org>.
yzh119 commented on a change in pull request #9748:
URL: https://github.com/apache/tvm/pull/9748#discussion_r787384558
##########
File path: include/tvm/runtime/container/optional.h
##########
@@ -93,6 +93,11 @@ class Optional : public ObjectRef {
ICHECK(data_ != nullptr);
return T(data_);
}
+ /*!
+ * \return The internal object pointer with type T* for Optional<T>.
+ * \note This function do not perform not-null checking.
+ */
+ const T* get() const { return data_; }
Review comment:
Thanks! yep, I just noticed the the type difference.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on a change in pull request #9748: Overload get() function for `Optional` type.
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #9748:
URL: https://github.com/apache/tvm/pull/9748#discussion_r787379719
##########
File path: include/tvm/runtime/container/optional.h
##########
@@ -93,6 +93,11 @@ class Optional : public ObjectRef {
ICHECK(data_ != nullptr);
return T(data_);
}
+ /*!
+ * \return The internal object pointer with type T* for Optional<T>.
+ * \note This function do not perform not-null checking.
+ */
+ const T* get() const { return data_; }
Review comment:
The type of `T` here is an ObjectRef, instead we should use `ContainerType` as defined a few lines above
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on a change in pull request #9748: Overload get() function for `Optional` type.
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #9748:
URL: https://github.com/apache/tvm/pull/9748#discussion_r808161589
##########
File path: include/tvm/runtime/container/optional.h
##########
@@ -93,6 +93,11 @@ class Optional : public ObjectRef {
ICHECK(data_ != nullptr);
return T(data_);
}
+ /*!
+ * \return The internal object pointer with container type of T.
+ * \note This function do not perform not-null checking.
+ */
+ const ContainerType* get() const { return static_cast<ContainerType*>(data_.get()); }
Review comment:
Indeed, I didn't noticed that ObjectRef also have a get method. I think this sounds good to me then. Perhaps we can just add additional comments about the behavior
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 merged pull request #9748: Overload get() function for `Optional` type.
Posted by GitBox <gi...@apache.org>.
junrushao1994 merged pull request #9748:
URL: https://github.com/apache/tvm/pull/9748
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org