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 2020/04/12 22:08:12 UTC
[GitHub] [incubator-tvm] tqchen opened a new pull request #5314: [IR] Allow
non-nullable ObjectRef, introduce Optional.
tqchen opened a new pull request #5314: [IR] Allow non-nullable ObjectRef, introduce Optional<T>.
URL: https://github.com/apache/incubator-tvm/pull/5314
We use ObjectRef and their sub-classes extensively throughout our codebase.
Each of ObjectRef's sub-classes are nullable, which means they can hold nullptr
as their values.
While in some places we need nullptr as an alternative value. The implicit support
for nullptr in all ObjectRef creates additional burdens for the developer
to explicitly check defined in many places of the codebase.
Moreover, it is unclear from the API's intentional point of view whether
we want a nullable object or not-null version(many cases we want the later).
Borrowing existing wisdoms from languages like Rust. We propose to
introduce non-nullable ObjectRef, and Optional<T> container that
represents a nullable variant.
To keep backward compatiblity, we will start by allowing most ObjectRef to be nullable.
However, we should start to use Optional<T> as the type in places where
we know nullable is a requirement. Gradually, we will move most of the ObjectRef
to be non-nullable and use Optional<T> in the nullable cases.
Such explicitness in typing can help reduce the potential problems
in our codebase overall.
Changes in this PR:
- Introduce _type_is_nullable attribute to ObjectRef
- Introduce Optional<T>
- Change String to be non-nullable.
- Change the API of function->GetAttr to return Optional<T>
Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @ them in the pull request thread.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5314:
[RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional.
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #5314: [RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional<T>.
URL: https://github.com/apache/incubator-tvm/pull/5314#discussion_r407298588
##########
File path: include/tvm/runtime/container.h
##########
@@ -610,7 +611,136 @@ struct PackedFuncValueConverter<::tvm::runtime::String> {
}
};
+/*!
+ * \brief Optional container that to represent to a Nullable variant of T.
+ * \tparam T The original ObjectRef.
+ *
+ * \code
+ *
+ * Optional<String> opt0 = nullptr;
+ * Optional<String> opt1 = String("xyz");
+ * CHECK(opt0 == nullptr);
+ * CHECK(opt1 == "xyz");
+ *
+ * \endcode
+ */
+template<typename T>
+class Optional : public ObjectRef {
+ public:
+ using ContainerType = typename T::ContainerType;
+ static_assert(std::is_base_of<ObjectRef, T>::value,
+ "Optional is only defined for ObjectRef.");
+ // default constructors.
+ Optional() = default;
+ Optional(const Optional<T>&) = default;
+ Optional(Optional<T>&&) = default;
+ Optional<T>& operator=(const Optional<T>&) = default;
+ Optional<T>& operator=(Optional<T>&&) = default;
+ /*!
+ * \brief Construct from an ObjectPtr
+ * whose type already matches the ContainerType.
+ * \param ptr
+ */
+ explicit Optional(ObjectPtr<Object> ptr) : ObjectRef(ptr) {}
+ // nullptr handling.
+ // disallow implicit conversion as 0 can be implicitly converted to nullptr_t
+ explicit Optional(std::nullptr_t) {}
+ Optional<T>& operator=(std::nullptr_t) {
+ data_ = nullptr;
+ return *this;
+ }
+ // normal value handling.
+ Optional(T other) // NOLINT(*)
+ : ObjectRef(std::move(other)) {
+ }
+ Optional<T>& operator=(T other) {
+ ObjectRef::operator=(std::move(other));
+ return *this;
+ }
+ // delete the int constructor
+ // since Optional<Integer>(0) is ambiguious
+ // 0 can be implicitly casted to nullptr_t
+ explicit Optional(int val) = delete;
+ Optional<T>& operator=(int val) = delete;
+ /*!
+ * \return A not-null container value in the optional.
+ * \note This function performs not-null checking.
+ */
+ T value() const {
+ CHECK(data_ != nullptr);
+ return T(data_);
+ }
+ /*!
+ * \return The contained value if the Optional is not null
+ * otherwise return the default_value.
+ */
+ T value_or(T default_value) const {
+ return data_ != nullptr ? T(data_) : default_value;
+ }
+ /*! \return Whether the container is not nullptr.*/
+ explicit operator bool() const {
+ return *this != nullptr;
+ }
+ // operator overloadings
+ bool operator==(std::nullptr_t) const {
+ return data_ == nullptr;
+ }
+ bool operator!=(std::nullptr_t) const {
+ return data_ != nullptr;
+ }
+ auto operator==(const Optional<T>& other) const {
+ // support case where sub-class returns a symbolic ref type.
+ using RetType = decltype(value() == other.value());
+ if (same_as(other)) return RetType(true);
+ if (*this != nullptr && other != nullptr) {
+ return value() == other.value();
+ } else {
+ // one of them is nullptr.
+ return RetType(false);
+ }
+ }
+ auto operator!=(const Optional<T>& other) const {
+ return !(*this == other);
+ }
+ auto operator==(const T& other) const {
+ using RetType = decltype(value() == other);
+ if (same_as(other)) return RetType(true);
+ if (*this != nullptr) return value() == other;
+ return RetType(false);
+ }
+ auto operator!=(const T& other) const {
+ return !(*this == other);
Review comment:
Here we assumed that the operator! is correctly overloaded for the return value of `value() == other`, which is the case for Bool
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5314:
[RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional.
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #5314: [RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional<T>.
URL: https://github.com/apache/incubator-tvm/pull/5314#discussion_r407260426
##########
File path: include/tvm/ir/attrs.h
##########
@@ -85,6 +85,8 @@ namespace tvm {
*/
template<typename TObjectRef>
inline TObjectRef NullValue() {
+ static_assert(TObjectRef::_type_is_nullable,
+ "Can only value for nullable types");
Review comment:
Please rephrase this error message.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] jroesch commented on issue #5314: [RUNTIME][IR]
Allow non-nullable ObjectRef, introduce Optional.
Posted by GitBox <gi...@apache.org>.
jroesch commented on issue #5314: [RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional<T>.
URL: https://github.com/apache/incubator-tvm/pull/5314#issuecomment-612812769
I'll review Monday morning.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5314:
[RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional.
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #5314: [RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional<T>.
URL: https://github.com/apache/incubator-tvm/pull/5314#discussion_r407301386
##########
File path: include/tvm/runtime/container.h
##########
@@ -610,7 +611,136 @@ struct PackedFuncValueConverter<::tvm::runtime::String> {
}
};
+/*!
+ * \brief Optional container that to represent to a Nullable variant of T.
+ * \tparam T The original ObjectRef.
+ *
+ * \code
+ *
+ * Optional<String> opt0 = nullptr;
+ * Optional<String> opt1 = String("xyz");
+ * CHECK(opt0 == nullptr);
+ * CHECK(opt1 == "xyz");
+ *
+ * \endcode
+ */
+template<typename T>
+class Optional : public ObjectRef {
+ public:
+ using ContainerType = typename T::ContainerType;
+ static_assert(std::is_base_of<ObjectRef, T>::value,
+ "Optional is only defined for ObjectRef.");
+ // default constructors.
+ Optional() = default;
+ Optional(const Optional<T>&) = default;
+ Optional(Optional<T>&&) = default;
+ Optional<T>& operator=(const Optional<T>&) = default;
+ Optional<T>& operator=(Optional<T>&&) = default;
+ /*!
+ * \brief Construct from an ObjectPtr
+ * whose type already matches the ContainerType.
+ * \param ptr
+ */
+ explicit Optional(ObjectPtr<Object> ptr) : ObjectRef(ptr) {}
+ // nullptr handling.
+ // disallow implicit conversion as 0 can be implicitly converted to nullptr_t
+ explicit Optional(std::nullptr_t) {}
+ Optional<T>& operator=(std::nullptr_t) {
+ data_ = nullptr;
+ return *this;
+ }
+ // normal value handling.
+ Optional(T other) // NOLINT(*)
+ : ObjectRef(std::move(other)) {
+ }
+ Optional<T>& operator=(T other) {
+ ObjectRef::operator=(std::move(other));
+ return *this;
+ }
+ // delete the int constructor
+ // since Optional<Integer>(0) is ambiguious
+ // 0 can be implicitly casted to nullptr_t
+ explicit Optional(int val) = delete;
+ Optional<T>& operator=(int val) = delete;
+ /*!
+ * \return A not-null container value in the optional.
+ * \note This function performs not-null checking.
+ */
+ T value() const {
+ CHECK(data_ != nullptr);
+ return T(data_);
+ }
+ /*!
+ * \return The contained value if the Optional is not null
+ * otherwise return the default_value.
+ */
+ T value_or(T default_value) const {
+ return data_ != nullptr ? T(data_) : default_value;
+ }
+ /*! \return Whether the container is not nullptr.*/
+ explicit operator bool() const {
+ return *this != nullptr;
+ }
+ // operator overloadings
+ bool operator==(std::nullptr_t) const {
+ return data_ == nullptr;
+ }
+ bool operator!=(std::nullptr_t) const {
+ return data_ != nullptr;
+ }
+ auto operator==(const Optional<T>& other) const {
+ // support case where sub-class returns a symbolic ref type.
+ using RetType = decltype(value() == other.value());
+ if (same_as(other)) return RetType(true);
+ if (*this != nullptr && other != nullptr) {
+ return value() == other.value();
+ } else {
+ // one of them is nullptr.
+ return RetType(false);
+ }
+ }
+ auto operator!=(const Optional<T>& other) const {
+ return !(*this == other);
+ }
+ auto operator==(const T& other) const {
+ using RetType = decltype(value() == other);
+ if (same_as(other)) return RetType(true);
+ if (*this != nullptr) return value() == other;
+ return RetType(false);
+ }
+ auto operator!=(const T& other) const {
+ return !(*this == other);
Review comment:
I added the suggested change, since it can potentially saves one indirection in the Bool creation.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] junrushao1994 commented on a change in pull request
#5314: [RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional.
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #5314: [RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional<T>.
URL: https://github.com/apache/incubator-tvm/pull/5314#discussion_r407296238
##########
File path: include/tvm/runtime/container.h
##########
@@ -610,7 +611,136 @@ struct PackedFuncValueConverter<::tvm::runtime::String> {
}
};
+/*!
+ * \brief Optional container that to represent to a Nullable variant of T.
+ * \tparam T The original ObjectRef.
+ *
+ * \code
+ *
+ * Optional<String> opt0 = nullptr;
+ * Optional<String> opt1 = String("xyz");
+ * CHECK(opt0 == nullptr);
+ * CHECK(opt1 == "xyz");
+ *
+ * \endcode
+ */
+template<typename T>
+class Optional : public ObjectRef {
+ public:
+ using ContainerType = typename T::ContainerType;
+ static_assert(std::is_base_of<ObjectRef, T>::value,
+ "Optional is only defined for ObjectRef.");
+ // default constructors.
+ Optional() = default;
+ Optional(const Optional<T>&) = default;
+ Optional(Optional<T>&&) = default;
+ Optional<T>& operator=(const Optional<T>&) = default;
+ Optional<T>& operator=(Optional<T>&&) = default;
+ /*!
+ * \brief Construct from an ObjectPtr
+ * whose type already matches the ContainerType.
+ * \param ptr
+ */
+ explicit Optional(ObjectPtr<Object> ptr) : ObjectRef(ptr) {}
+ // nullptr handling.
+ // disallow implicit conversion as 0 can be implicitly converted to nullptr_t
+ explicit Optional(std::nullptr_t) {}
+ Optional<T>& operator=(std::nullptr_t) {
+ data_ = nullptr;
+ return *this;
+ }
+ // normal value handling.
+ Optional(T other) // NOLINT(*)
+ : ObjectRef(std::move(other)) {
+ }
+ Optional<T>& operator=(T other) {
+ ObjectRef::operator=(std::move(other));
+ return *this;
+ }
+ // delete the int constructor
+ // since Optional<Integer>(0) is ambiguious
+ // 0 can be implicitly casted to nullptr_t
+ explicit Optional(int val) = delete;
+ Optional<T>& operator=(int val) = delete;
+ /*!
+ * \return A not-null container value in the optional.
+ * \note This function performs not-null checking.
+ */
+ T value() const {
+ CHECK(data_ != nullptr);
+ return T(data_);
+ }
+ /*!
+ * \return The contained value if the Optional is not null
+ * otherwise return the default_value.
+ */
+ T value_or(T default_value) const {
+ return data_ != nullptr ? T(data_) : default_value;
+ }
+ /*! \return Whether the container is not nullptr.*/
+ explicit operator bool() const {
+ return *this != nullptr;
+ }
+ // operator overloadings
+ bool operator==(std::nullptr_t) const {
+ return data_ == nullptr;
+ }
+ bool operator!=(std::nullptr_t) const {
+ return data_ != nullptr;
+ }
+ auto operator==(const Optional<T>& other) const {
+ // support case where sub-class returns a symbolic ref type.
+ using RetType = decltype(value() == other.value());
+ if (same_as(other)) return RetType(true);
+ if (*this != nullptr && other != nullptr) {
+ return value() == other.value();
+ } else {
+ // one of them is nullptr.
+ return RetType(false);
+ }
+ }
+ auto operator!=(const Optional<T>& other) const {
+ return !(*this == other);
+ }
+ auto operator==(const T& other) const {
+ using RetType = decltype(value() == other);
+ if (same_as(other)) return RetType(true);
+ if (*this != nullptr) return value() == other;
+ return RetType(false);
+ }
+ auto operator!=(const T& other) const {
+ return !(*this == other);
Review comment:
This would be better to use the same logic as in "operator ==", because we didn't assume that `decltype(value() == other)` is boolean, so it might be slightly better not to directly apply `!` to the result.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen edited a comment on issue #5314:
[RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional.
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on issue #5314: [RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional<T>.
URL: https://github.com/apache/incubator-tvm/pull/5314#issuecomment-612683588
related dicussion thread https://discuss.tvm.ai/t/allow-non-nullable-object-and-introduce-optional-t/6337/
cc @jroesch @zhiics @wweic @MarisaKirisame @yzhliu @junrushao1994
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] MarisaKirisame commented on a change in pull
request #5314: [RUNTIME][IR] Allow non-nullable ObjectRef,
introduce Optional.
Posted by GitBox <gi...@apache.org>.
MarisaKirisame commented on a change in pull request #5314: [RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional<T>.
URL: https://github.com/apache/incubator-tvm/pull/5314#discussion_r407427828
##########
File path: include/tvm/runtime/container.h
##########
@@ -610,7 +611,146 @@ struct PackedFuncValueConverter<::tvm::runtime::String> {
}
};
+/*!
+ * \brief Optional container that to represent to a Nullable variant of T.
+ * \tparam T The original ObjectRef.
+ *
+ * \code
+ *
+ * Optional<String> opt0 = nullptr;
+ * Optional<String> opt1 = String("xyz");
+ * CHECK(opt0 == nullptr);
+ * CHECK(opt1 == "xyz");
+ *
+ * \endcode
+ */
+template<typename T>
+class Optional : public ObjectRef {
+ public:
+ using ContainerType = typename T::ContainerType;
+ static_assert(std::is_base_of<ObjectRef, T>::value,
+ "Optional is only defined for ObjectRef.");
+ // default constructors.
+ Optional() = default;
+ Optional(const Optional<T>&) = default;
+ Optional(Optional<T>&&) = default;
+ Optional<T>& operator=(const Optional<T>&) = default;
+ Optional<T>& operator=(Optional<T>&&) = default;
+ /*!
+ * \brief Construct from an ObjectPtr
+ * whose type already matches the ContainerType.
+ * \param ptr
+ */
+ explicit Optional(ObjectPtr<Object> ptr) : ObjectRef(ptr) {}
+ // nullptr handling.
+ // disallow implicit conversion as 0 can be implicitly converted to nullptr_t
+ explicit Optional(std::nullptr_t) {}
+ Optional<T>& operator=(std::nullptr_t) {
+ data_ = nullptr;
+ return *this;
+ }
+ // normal value handling.
+ Optional(T other) // NOLINT(*)
+ : ObjectRef(std::move(other)) {
+ }
+ Optional<T>& operator=(T other) {
+ ObjectRef::operator=(std::move(other));
+ return *this;
+ }
+ // delete the int constructor
+ // since Optional<Integer>(0) is ambiguious
+ // 0 can be implicitly casted to nullptr_t
+ explicit Optional(int val) = delete;
+ Optional<T>& operator=(int val) = delete;
+ /*!
+ * \return A not-null container value in the optional.
+ * \note This function performs not-null checking.
+ */
+ T value() const {
+ CHECK(data_ != nullptr);
+ return T(data_);
+ }
+ /*!
+ * \return The contained value if the Optional is not null
+ * otherwise return the default_value.
+ */
+ T value_or(T default_value) const {
+ return data_ != nullptr ? T(data_) : default_value;
+ }
+ /*! \return Whether the container is not nullptr.*/
+ explicit operator bool() const {
Review comment:
I am quite against this operator overload. This will create confusion around the operator bool() of Optional<Bool>. I think it is best to name it .defined() and be a little bit cumbersome to use then to have this potential misuse.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5314:
[RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional.
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #5314: [RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional<T>.
URL: https://github.com/apache/incubator-tvm/pull/5314#discussion_r407463134
##########
File path: include/tvm/runtime/container.h
##########
@@ -610,7 +611,146 @@ struct PackedFuncValueConverter<::tvm::runtime::String> {
}
};
+/*!
+ * \brief Optional container that to represent to a Nullable variant of T.
+ * \tparam T The original ObjectRef.
+ *
+ * \code
+ *
+ * Optional<String> opt0 = nullptr;
+ * Optional<String> opt1 = String("xyz");
+ * CHECK(opt0 == nullptr);
+ * CHECK(opt1 == "xyz");
+ *
+ * \endcode
+ */
+template<typename T>
+class Optional : public ObjectRef {
+ public:
+ using ContainerType = typename T::ContainerType;
+ static_assert(std::is_base_of<ObjectRef, T>::value,
+ "Optional is only defined for ObjectRef.");
+ // default constructors.
+ Optional() = default;
+ Optional(const Optional<T>&) = default;
+ Optional(Optional<T>&&) = default;
+ Optional<T>& operator=(const Optional<T>&) = default;
+ Optional<T>& operator=(Optional<T>&&) = default;
+ /*!
+ * \brief Construct from an ObjectPtr
+ * whose type already matches the ContainerType.
+ * \param ptr
+ */
+ explicit Optional(ObjectPtr<Object> ptr) : ObjectRef(ptr) {}
+ // nullptr handling.
+ // disallow implicit conversion as 0 can be implicitly converted to nullptr_t
+ explicit Optional(std::nullptr_t) {}
+ Optional<T>& operator=(std::nullptr_t) {
+ data_ = nullptr;
+ return *this;
+ }
+ // normal value handling.
+ Optional(T other) // NOLINT(*)
+ : ObjectRef(std::move(other)) {
+ }
+ Optional<T>& operator=(T other) {
+ ObjectRef::operator=(std::move(other));
+ return *this;
+ }
+ // delete the int constructor
+ // since Optional<Integer>(0) is ambiguious
+ // 0 can be implicitly casted to nullptr_t
+ explicit Optional(int val) = delete;
+ Optional<T>& operator=(int val) = delete;
+ /*!
+ * \return A not-null container value in the optional.
+ * \note This function performs not-null checking.
+ */
+ T value() const {
+ CHECK(data_ != nullptr);
+ return T(data_);
+ }
+ /*!
+ * \return The contained value if the Optional is not null
+ * otherwise return the default_value.
+ */
+ T value_or(T default_value) const {
Review comment:
It depends on whether we take the default or non-default path. If we take the default pass, passing by value is better than pass by argument. Perhaps we can add an overload to both cases later
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen merged pull request #5314: [RUNTIME][IR]
Allow non-nullable ObjectRef, introduce Optional.
Posted by GitBox <gi...@apache.org>.
tqchen merged pull request #5314: [RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional<T>.
URL: https://github.com/apache/incubator-tvm/pull/5314
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] junrushao1994 commented on a change in pull request
#5314: [RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional.
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #5314: [RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional<T>.
URL: https://github.com/apache/incubator-tvm/pull/5314#discussion_r407299850
##########
File path: include/tvm/runtime/container.h
##########
@@ -610,7 +611,136 @@ struct PackedFuncValueConverter<::tvm::runtime::String> {
}
};
+/*!
+ * \brief Optional container that to represent to a Nullable variant of T.
+ * \tparam T The original ObjectRef.
+ *
+ * \code
+ *
+ * Optional<String> opt0 = nullptr;
+ * Optional<String> opt1 = String("xyz");
+ * CHECK(opt0 == nullptr);
+ * CHECK(opt1 == "xyz");
+ *
+ * \endcode
+ */
+template<typename T>
+class Optional : public ObjectRef {
+ public:
+ using ContainerType = typename T::ContainerType;
+ static_assert(std::is_base_of<ObjectRef, T>::value,
+ "Optional is only defined for ObjectRef.");
+ // default constructors.
+ Optional() = default;
+ Optional(const Optional<T>&) = default;
+ Optional(Optional<T>&&) = default;
+ Optional<T>& operator=(const Optional<T>&) = default;
+ Optional<T>& operator=(Optional<T>&&) = default;
+ /*!
+ * \brief Construct from an ObjectPtr
+ * whose type already matches the ContainerType.
+ * \param ptr
+ */
+ explicit Optional(ObjectPtr<Object> ptr) : ObjectRef(ptr) {}
+ // nullptr handling.
+ // disallow implicit conversion as 0 can be implicitly converted to nullptr_t
+ explicit Optional(std::nullptr_t) {}
+ Optional<T>& operator=(std::nullptr_t) {
+ data_ = nullptr;
+ return *this;
+ }
+ // normal value handling.
+ Optional(T other) // NOLINT(*)
+ : ObjectRef(std::move(other)) {
+ }
+ Optional<T>& operator=(T other) {
+ ObjectRef::operator=(std::move(other));
+ return *this;
+ }
+ // delete the int constructor
+ // since Optional<Integer>(0) is ambiguious
+ // 0 can be implicitly casted to nullptr_t
+ explicit Optional(int val) = delete;
+ Optional<T>& operator=(int val) = delete;
+ /*!
+ * \return A not-null container value in the optional.
+ * \note This function performs not-null checking.
+ */
+ T value() const {
+ CHECK(data_ != nullptr);
+ return T(data_);
+ }
+ /*!
+ * \return The contained value if the Optional is not null
+ * otherwise return the default_value.
+ */
+ T value_or(T default_value) const {
+ return data_ != nullptr ? T(data_) : default_value;
+ }
+ /*! \return Whether the container is not nullptr.*/
+ explicit operator bool() const {
+ return *this != nullptr;
+ }
+ // operator overloadings
+ bool operator==(std::nullptr_t) const {
+ return data_ == nullptr;
+ }
+ bool operator!=(std::nullptr_t) const {
+ return data_ != nullptr;
+ }
+ auto operator==(const Optional<T>& other) const {
+ // support case where sub-class returns a symbolic ref type.
+ using RetType = decltype(value() == other.value());
+ if (same_as(other)) return RetType(true);
+ if (*this != nullptr && other != nullptr) {
+ return value() == other.value();
+ } else {
+ // one of them is nullptr.
+ return RetType(false);
+ }
+ }
+ auto operator!=(const Optional<T>& other) const {
+ return !(*this == other);
+ }
+ auto operator==(const T& other) const {
+ using RetType = decltype(value() == other);
+ if (same_as(other)) return RetType(true);
+ if (*this != nullptr) return value() == other;
+ return RetType(false);
+ }
+ auto operator!=(const T& other) const {
+ return !(*this == other);
Review comment:
I don't have strong option on this - it is fine for Bool to do either way.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] MarisaKirisame commented on a change in pull
request #5314: [RUNTIME][IR] Allow non-nullable ObjectRef,
introduce Optional.
Posted by GitBox <gi...@apache.org>.
MarisaKirisame commented on a change in pull request #5314: [RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional<T>.
URL: https://github.com/apache/incubator-tvm/pull/5314#discussion_r407428939
##########
File path: include/tvm/runtime/container.h
##########
@@ -610,7 +611,146 @@ struct PackedFuncValueConverter<::tvm::runtime::String> {
}
};
+/*!
+ * \brief Optional container that to represent to a Nullable variant of T.
+ * \tparam T The original ObjectRef.
+ *
+ * \code
+ *
+ * Optional<String> opt0 = nullptr;
+ * Optional<String> opt1 = String("xyz");
+ * CHECK(opt0 == nullptr);
+ * CHECK(opt1 == "xyz");
+ *
+ * \endcode
+ */
+template<typename T>
+class Optional : public ObjectRef {
+ public:
+ using ContainerType = typename T::ContainerType;
+ static_assert(std::is_base_of<ObjectRef, T>::value,
+ "Optional is only defined for ObjectRef.");
+ // default constructors.
+ Optional() = default;
+ Optional(const Optional<T>&) = default;
+ Optional(Optional<T>&&) = default;
+ Optional<T>& operator=(const Optional<T>&) = default;
+ Optional<T>& operator=(Optional<T>&&) = default;
+ /*!
+ * \brief Construct from an ObjectPtr
+ * whose type already matches the ContainerType.
+ * \param ptr
+ */
+ explicit Optional(ObjectPtr<Object> ptr) : ObjectRef(ptr) {}
+ // nullptr handling.
+ // disallow implicit conversion as 0 can be implicitly converted to nullptr_t
+ explicit Optional(std::nullptr_t) {}
+ Optional<T>& operator=(std::nullptr_t) {
+ data_ = nullptr;
+ return *this;
+ }
+ // normal value handling.
+ Optional(T other) // NOLINT(*)
+ : ObjectRef(std::move(other)) {
+ }
+ Optional<T>& operator=(T other) {
+ ObjectRef::operator=(std::move(other));
+ return *this;
+ }
+ // delete the int constructor
+ // since Optional<Integer>(0) is ambiguious
+ // 0 can be implicitly casted to nullptr_t
+ explicit Optional(int val) = delete;
+ Optional<T>& operator=(int val) = delete;
+ /*!
+ * \return A not-null container value in the optional.
+ * \note This function performs not-null checking.
+ */
+ T value() const {
+ CHECK(data_ != nullptr);
+ return T(data_);
+ }
+ /*!
+ * \return The contained value if the Optional is not null
+ * otherwise return the default_value.
+ */
+ T value_or(T default_value) const {
Review comment:
const& to avoid unnecessary copying?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5314:
[RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional.
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #5314: [RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional<T>.
URL: https://github.com/apache/incubator-tvm/pull/5314#discussion_r407462403
##########
File path: include/tvm/runtime/container.h
##########
@@ -610,7 +611,146 @@ struct PackedFuncValueConverter<::tvm::runtime::String> {
}
};
+/*!
+ * \brief Optional container that to represent to a Nullable variant of T.
+ * \tparam T The original ObjectRef.
+ *
+ * \code
+ *
+ * Optional<String> opt0 = nullptr;
+ * Optional<String> opt1 = String("xyz");
+ * CHECK(opt0 == nullptr);
+ * CHECK(opt1 == "xyz");
+ *
+ * \endcode
+ */
+template<typename T>
+class Optional : public ObjectRef {
+ public:
+ using ContainerType = typename T::ContainerType;
+ static_assert(std::is_base_of<ObjectRef, T>::value,
+ "Optional is only defined for ObjectRef.");
+ // default constructors.
+ Optional() = default;
+ Optional(const Optional<T>&) = default;
+ Optional(Optional<T>&&) = default;
+ Optional<T>& operator=(const Optional<T>&) = default;
+ Optional<T>& operator=(Optional<T>&&) = default;
+ /*!
+ * \brief Construct from an ObjectPtr
+ * whose type already matches the ContainerType.
+ * \param ptr
+ */
+ explicit Optional(ObjectPtr<Object> ptr) : ObjectRef(ptr) {}
+ // nullptr handling.
+ // disallow implicit conversion as 0 can be implicitly converted to nullptr_t
+ explicit Optional(std::nullptr_t) {}
+ Optional<T>& operator=(std::nullptr_t) {
+ data_ = nullptr;
+ return *this;
+ }
+ // normal value handling.
+ Optional(T other) // NOLINT(*)
+ : ObjectRef(std::move(other)) {
+ }
+ Optional<T>& operator=(T other) {
+ ObjectRef::operator=(std::move(other));
+ return *this;
+ }
+ // delete the int constructor
+ // since Optional<Integer>(0) is ambiguious
+ // 0 can be implicitly casted to nullptr_t
+ explicit Optional(int val) = delete;
+ Optional<T>& operator=(int val) = delete;
+ /*!
+ * \return A not-null container value in the optional.
+ * \note This function performs not-null checking.
+ */
+ T value() const {
+ CHECK(data_ != nullptr);
+ return T(data_);
+ }
+ /*!
+ * \return The contained value if the Optional is not null
+ * otherwise return the default_value.
+ */
+ T value_or(T default_value) const {
+ return data_ != nullptr ? T(data_) : default_value;
+ }
+ /*! \return Whether the container is not nullptr.*/
+ explicit operator bool() const {
Review comment:
This overload is needed to enable sugar
```c++
if (auto opt = GetSomeOptional) {
// code
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] junrushao1994 commented on a change in pull request
#5314: [RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional.
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #5314: [RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional<T>.
URL: https://github.com/apache/incubator-tvm/pull/5314#discussion_r407296238
##########
File path: include/tvm/runtime/container.h
##########
@@ -610,7 +611,136 @@ struct PackedFuncValueConverter<::tvm::runtime::String> {
}
};
+/*!
+ * \brief Optional container that to represent to a Nullable variant of T.
+ * \tparam T The original ObjectRef.
+ *
+ * \code
+ *
+ * Optional<String> opt0 = nullptr;
+ * Optional<String> opt1 = String("xyz");
+ * CHECK(opt0 == nullptr);
+ * CHECK(opt1 == "xyz");
+ *
+ * \endcode
+ */
+template<typename T>
+class Optional : public ObjectRef {
+ public:
+ using ContainerType = typename T::ContainerType;
+ static_assert(std::is_base_of<ObjectRef, T>::value,
+ "Optional is only defined for ObjectRef.");
+ // default constructors.
+ Optional() = default;
+ Optional(const Optional<T>&) = default;
+ Optional(Optional<T>&&) = default;
+ Optional<T>& operator=(const Optional<T>&) = default;
+ Optional<T>& operator=(Optional<T>&&) = default;
+ /*!
+ * \brief Construct from an ObjectPtr
+ * whose type already matches the ContainerType.
+ * \param ptr
+ */
+ explicit Optional(ObjectPtr<Object> ptr) : ObjectRef(ptr) {}
+ // nullptr handling.
+ // disallow implicit conversion as 0 can be implicitly converted to nullptr_t
+ explicit Optional(std::nullptr_t) {}
+ Optional<T>& operator=(std::nullptr_t) {
+ data_ = nullptr;
+ return *this;
+ }
+ // normal value handling.
+ Optional(T other) // NOLINT(*)
+ : ObjectRef(std::move(other)) {
+ }
+ Optional<T>& operator=(T other) {
+ ObjectRef::operator=(std::move(other));
+ return *this;
+ }
+ // delete the int constructor
+ // since Optional<Integer>(0) is ambiguious
+ // 0 can be implicitly casted to nullptr_t
+ explicit Optional(int val) = delete;
+ Optional<T>& operator=(int val) = delete;
+ /*!
+ * \return A not-null container value in the optional.
+ * \note This function performs not-null checking.
+ */
+ T value() const {
+ CHECK(data_ != nullptr);
+ return T(data_);
+ }
+ /*!
+ * \return The contained value if the Optional is not null
+ * otherwise return the default_value.
+ */
+ T value_or(T default_value) const {
+ return data_ != nullptr ? T(data_) : default_value;
+ }
+ /*! \return Whether the container is not nullptr.*/
+ explicit operator bool() const {
+ return *this != nullptr;
+ }
+ // operator overloadings
+ bool operator==(std::nullptr_t) const {
+ return data_ == nullptr;
+ }
+ bool operator!=(std::nullptr_t) const {
+ return data_ != nullptr;
+ }
+ auto operator==(const Optional<T>& other) const {
+ // support case where sub-class returns a symbolic ref type.
+ using RetType = decltype(value() == other.value());
+ if (same_as(other)) return RetType(true);
+ if (*this != nullptr && other != nullptr) {
+ return value() == other.value();
+ } else {
+ // one of them is nullptr.
+ return RetType(false);
+ }
+ }
+ auto operator!=(const Optional<T>& other) const {
+ return !(*this == other);
+ }
+ auto operator==(const T& other) const {
+ using RetType = decltype(value() == other);
+ if (same_as(other)) return RetType(true);
+ if (*this != nullptr) return value() == other;
+ return RetType(false);
+ }
+ auto operator!=(const T& other) const {
+ return !(*this == other);
Review comment:
This would be better to use the same logic as in "operator ==", because we didn't assume that `decltype(value() == other)` is boolean
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on issue #5314: [RUNTIME][IR]
Allow non-nullable ObjectRef, introduce Optional.
Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #5314: [RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional<T>.
URL: https://github.com/apache/incubator-tvm/pull/5314#issuecomment-613012133
@MarisaKirisame we don't have to expose optional to python as we can directly use python's Optional for typing. When the normal object typing is used, i believe they are not nullable, the runtime python object themselves are nullable.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on issue #5314: [RUNTIME][IR]
Allow non-nullable ObjectRef, introduce Optional.
Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #5314: [RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional<T>.
URL: https://github.com/apache/incubator-tvm/pull/5314#issuecomment-612683588
cc @jroesch @zhiics @wweic @MarisaKirisame
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen edited a comment on issue #5314:
[RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional.
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on issue #5314: [RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional<T>.
URL: https://github.com/apache/incubator-tvm/pull/5314#issuecomment-612683588
related dicussion thread https://discuss.tvm.ai/t/allow-non-nullable-object-and-introduce-optional-t/6337/
cc @jroesch @zhiics @wweic @MarisaKirisame
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen edited a comment on issue #5314:
[RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional.
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on issue #5314: [RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional<T>.
URL: https://github.com/apache/incubator-tvm/pull/5314#issuecomment-612683588
related dicussion thread https://discuss.tvm.ai/t/allow-non-nullable-object-and-introduce-optional-t/6337/
cc @jroesch @zhiics @wweic @MarisaKirisame @yzhliu
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] junrushao1994 commented on a change in pull request
#5314: [RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional.
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #5314: [RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional<T>.
URL: https://github.com/apache/incubator-tvm/pull/5314#discussion_r407296238
##########
File path: include/tvm/runtime/container.h
##########
@@ -610,7 +611,136 @@ struct PackedFuncValueConverter<::tvm::runtime::String> {
}
};
+/*!
+ * \brief Optional container that to represent to a Nullable variant of T.
+ * \tparam T The original ObjectRef.
+ *
+ * \code
+ *
+ * Optional<String> opt0 = nullptr;
+ * Optional<String> opt1 = String("xyz");
+ * CHECK(opt0 == nullptr);
+ * CHECK(opt1 == "xyz");
+ *
+ * \endcode
+ */
+template<typename T>
+class Optional : public ObjectRef {
+ public:
+ using ContainerType = typename T::ContainerType;
+ static_assert(std::is_base_of<ObjectRef, T>::value,
+ "Optional is only defined for ObjectRef.");
+ // default constructors.
+ Optional() = default;
+ Optional(const Optional<T>&) = default;
+ Optional(Optional<T>&&) = default;
+ Optional<T>& operator=(const Optional<T>&) = default;
+ Optional<T>& operator=(Optional<T>&&) = default;
+ /*!
+ * \brief Construct from an ObjectPtr
+ * whose type already matches the ContainerType.
+ * \param ptr
+ */
+ explicit Optional(ObjectPtr<Object> ptr) : ObjectRef(ptr) {}
+ // nullptr handling.
+ // disallow implicit conversion as 0 can be implicitly converted to nullptr_t
+ explicit Optional(std::nullptr_t) {}
+ Optional<T>& operator=(std::nullptr_t) {
+ data_ = nullptr;
+ return *this;
+ }
+ // normal value handling.
+ Optional(T other) // NOLINT(*)
+ : ObjectRef(std::move(other)) {
+ }
+ Optional<T>& operator=(T other) {
+ ObjectRef::operator=(std::move(other));
+ return *this;
+ }
+ // delete the int constructor
+ // since Optional<Integer>(0) is ambiguious
+ // 0 can be implicitly casted to nullptr_t
+ explicit Optional(int val) = delete;
+ Optional<T>& operator=(int val) = delete;
+ /*!
+ * \return A not-null container value in the optional.
+ * \note This function performs not-null checking.
+ */
+ T value() const {
+ CHECK(data_ != nullptr);
+ return T(data_);
+ }
+ /*!
+ * \return The contained value if the Optional is not null
+ * otherwise return the default_value.
+ */
+ T value_or(T default_value) const {
+ return data_ != nullptr ? T(data_) : default_value;
+ }
+ /*! \return Whether the container is not nullptr.*/
+ explicit operator bool() const {
+ return *this != nullptr;
+ }
+ // operator overloadings
+ bool operator==(std::nullptr_t) const {
+ return data_ == nullptr;
+ }
+ bool operator!=(std::nullptr_t) const {
+ return data_ != nullptr;
+ }
+ auto operator==(const Optional<T>& other) const {
+ // support case where sub-class returns a symbolic ref type.
+ using RetType = decltype(value() == other.value());
+ if (same_as(other)) return RetType(true);
+ if (*this != nullptr && other != nullptr) {
+ return value() == other.value();
+ } else {
+ // one of them is nullptr.
+ return RetType(false);
+ }
+ }
+ auto operator!=(const Optional<T>& other) const {
+ return !(*this == other);
+ }
+ auto operator==(const T& other) const {
+ using RetType = decltype(value() == other);
+ if (same_as(other)) return RetType(true);
+ if (*this != nullptr) return value() == other;
+ return RetType(false);
+ }
+ auto operator!=(const T& other) const {
+ return !(*this == other);
Review comment:
This would be better to use the same logic as in "operator ==", because we didn't assume that `!decltype(value() == other)` is boolean
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5314:
[RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional.
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #5314: [RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional<T>.
URL: https://github.com/apache/incubator-tvm/pull/5314#discussion_r407263583
##########
File path: include/tvm/runtime/container.h
##########
@@ -610,7 +611,123 @@ struct PackedFuncValueConverter<::tvm::runtime::String> {
}
};
+/*!
+ * \brief Optional container that to represent to a Nullable variant of T.
+ * \tparam T The original ObjectRef.
+ *
+ * \code
+ *
+ * Optional<String> opt0 = nullptr;
+ * Optional<String> opt1 = String("xyz");
+ * CHECK(opt0 == nullptr);
+ * CHECK(opt1 == "xyz");
+ *
+ * \endcode
+ */
+template<typename T>
+class Optional : public ObjectRef {
+ public:
+ using ContainerType = typename T::ContainerType;
+ static_assert(std::is_base_of<ObjectRef, T>::value,
+ "Optional is only defined for ObjectRef.");
+ // default constructors.
+ Optional() = default;
+ Optional(const Optional<T>&) = default;
+ Optional(Optional<T>&&) = default;
+ Optional<T>& operator=(const Optional<T>&) = default;
+ Optional<T>& operator=(Optional<T>&&) = default;
+ /*!
+ * \brief Construct from an ObjectPtr
+ * whose type already matches the ContainerType.
+ * \param ptr
+ */
+ explicit Optional(ObjectPtr<Object> ptr) : ObjectRef(ptr) {}
+ // nullptr handling.
+ Optional(std::nullptr_t) {} // NOLINT(*)
+ Optional<T>& operator=(std::nullptr_t) {
+ data_ = nullptr;
+ return *this;
+ }
+ // normal value handling.
+ Optional(T other) // NOLINT(*)
+ : ObjectRef(std::move(other)) {
+ }
+ Optional<T>& operator=(T other) {
+ ObjectRef::operator=(std::move(other));
+ return *this;
+ }
+ /*!
+ * \return A not-null container value in the optional.
+ * \note This function performs not-null checking.
+ */
+ T value() const {
+ CHECK(data_ != nullptr);
+ return T(data_);
+ }
Review comment:
It might be also useful have value_or as std::optional provides? This way we can return a "default" value.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5314:
[RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional.
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #5314: [RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional<T>.
URL: https://github.com/apache/incubator-tvm/pull/5314#discussion_r407260236
##########
File path: tests/cpp/container_test.cc
##########
@@ -401,6 +401,58 @@ TEST(String, Cast) {
String s2 = Downcast<String>(r);
}
+
+TEST(Optional, Composition) {
+ Optional<String> opt0 = nullptr;
+ Optional<String> opt1 = String("xyz");
+ Optional<String> opt2 = String("xyz1");
+ // operator bool
+ CHECK(!opt0);
+ CHECK(opt1);
+ // comparison op
+ CHECK(opt0 != "xyz");
+ CHECK(opt1 == "xyz");
+ CHECK(opt1 != nullptr);
+ CHECK(opt0 == nullptr);
+ CHECK(opt0 != opt1);
+ CHECK(opt1 == Optional<String>(String("xyz")));
+ CHECK(opt0 == Optional<String>(nullptr));
+ opt0 = opt1;
+ CHECK(opt0 == opt1);
+ CHECK(opt0.value().same_as(opt1.value()));
+ opt0 = std::move(opt2);
+ CHECK(opt0 != opt2);
+}
+
+TEST(Optional, PackedCall) {
+ auto tf = [](Optional<String> s, bool isnull) {
+ if (isnull) {
+ CHECK(s == nullptr);
+ } else {
+ CHECK(s != nullptr);
+ }
+ return s;
+ };
+ auto func = TypedPackedFunc<Optional<String>(Optional<String>, bool)>(tf);
+ func(String("xyz"), false);
Review comment:
Check the returned value for these funcs?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5314:
[RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional.
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #5314: [RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional<T>.
URL: https://github.com/apache/incubator-tvm/pull/5314#discussion_r407301386
##########
File path: include/tvm/runtime/container.h
##########
@@ -610,7 +611,136 @@ struct PackedFuncValueConverter<::tvm::runtime::String> {
}
};
+/*!
+ * \brief Optional container that to represent to a Nullable variant of T.
+ * \tparam T The original ObjectRef.
+ *
+ * \code
+ *
+ * Optional<String> opt0 = nullptr;
+ * Optional<String> opt1 = String("xyz");
+ * CHECK(opt0 == nullptr);
+ * CHECK(opt1 == "xyz");
+ *
+ * \endcode
+ */
+template<typename T>
+class Optional : public ObjectRef {
+ public:
+ using ContainerType = typename T::ContainerType;
+ static_assert(std::is_base_of<ObjectRef, T>::value,
+ "Optional is only defined for ObjectRef.");
+ // default constructors.
+ Optional() = default;
+ Optional(const Optional<T>&) = default;
+ Optional(Optional<T>&&) = default;
+ Optional<T>& operator=(const Optional<T>&) = default;
+ Optional<T>& operator=(Optional<T>&&) = default;
+ /*!
+ * \brief Construct from an ObjectPtr
+ * whose type already matches the ContainerType.
+ * \param ptr
+ */
+ explicit Optional(ObjectPtr<Object> ptr) : ObjectRef(ptr) {}
+ // nullptr handling.
+ // disallow implicit conversion as 0 can be implicitly converted to nullptr_t
+ explicit Optional(std::nullptr_t) {}
+ Optional<T>& operator=(std::nullptr_t) {
+ data_ = nullptr;
+ return *this;
+ }
+ // normal value handling.
+ Optional(T other) // NOLINT(*)
+ : ObjectRef(std::move(other)) {
+ }
+ Optional<T>& operator=(T other) {
+ ObjectRef::operator=(std::move(other));
+ return *this;
+ }
+ // delete the int constructor
+ // since Optional<Integer>(0) is ambiguious
+ // 0 can be implicitly casted to nullptr_t
+ explicit Optional(int val) = delete;
+ Optional<T>& operator=(int val) = delete;
+ /*!
+ * \return A not-null container value in the optional.
+ * \note This function performs not-null checking.
+ */
+ T value() const {
+ CHECK(data_ != nullptr);
+ return T(data_);
+ }
+ /*!
+ * \return The contained value if the Optional is not null
+ * otherwise return the default_value.
+ */
+ T value_or(T default_value) const {
+ return data_ != nullptr ? T(data_) : default_value;
+ }
+ /*! \return Whether the container is not nullptr.*/
+ explicit operator bool() const {
+ return *this != nullptr;
+ }
+ // operator overloadings
+ bool operator==(std::nullptr_t) const {
+ return data_ == nullptr;
+ }
+ bool operator!=(std::nullptr_t) const {
+ return data_ != nullptr;
+ }
+ auto operator==(const Optional<T>& other) const {
+ // support case where sub-class returns a symbolic ref type.
+ using RetType = decltype(value() == other.value());
+ if (same_as(other)) return RetType(true);
+ if (*this != nullptr && other != nullptr) {
+ return value() == other.value();
+ } else {
+ // one of them is nullptr.
+ return RetType(false);
+ }
+ }
+ auto operator!=(const Optional<T>& other) const {
+ return !(*this == other);
+ }
+ auto operator==(const T& other) const {
+ using RetType = decltype(value() == other);
+ if (same_as(other)) return RetType(true);
+ if (*this != nullptr) return value() == other;
+ return RetType(false);
+ }
+ auto operator!=(const T& other) const {
+ return !(*this == other);
Review comment:
I added the change, since it can potentially saves one indirection in the Bool creation.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5314:
[RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional.
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #5314: [RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional<T>.
URL: https://github.com/apache/incubator-tvm/pull/5314#discussion_r407463134
##########
File path: include/tvm/runtime/container.h
##########
@@ -610,7 +611,146 @@ struct PackedFuncValueConverter<::tvm::runtime::String> {
}
};
+/*!
+ * \brief Optional container that to represent to a Nullable variant of T.
+ * \tparam T The original ObjectRef.
+ *
+ * \code
+ *
+ * Optional<String> opt0 = nullptr;
+ * Optional<String> opt1 = String("xyz");
+ * CHECK(opt0 == nullptr);
+ * CHECK(opt1 == "xyz");
+ *
+ * \endcode
+ */
+template<typename T>
+class Optional : public ObjectRef {
+ public:
+ using ContainerType = typename T::ContainerType;
+ static_assert(std::is_base_of<ObjectRef, T>::value,
+ "Optional is only defined for ObjectRef.");
+ // default constructors.
+ Optional() = default;
+ Optional(const Optional<T>&) = default;
+ Optional(Optional<T>&&) = default;
+ Optional<T>& operator=(const Optional<T>&) = default;
+ Optional<T>& operator=(Optional<T>&&) = default;
+ /*!
+ * \brief Construct from an ObjectPtr
+ * whose type already matches the ContainerType.
+ * \param ptr
+ */
+ explicit Optional(ObjectPtr<Object> ptr) : ObjectRef(ptr) {}
+ // nullptr handling.
+ // disallow implicit conversion as 0 can be implicitly converted to nullptr_t
+ explicit Optional(std::nullptr_t) {}
+ Optional<T>& operator=(std::nullptr_t) {
+ data_ = nullptr;
+ return *this;
+ }
+ // normal value handling.
+ Optional(T other) // NOLINT(*)
+ : ObjectRef(std::move(other)) {
+ }
+ Optional<T>& operator=(T other) {
+ ObjectRef::operator=(std::move(other));
+ return *this;
+ }
+ // delete the int constructor
+ // since Optional<Integer>(0) is ambiguious
+ // 0 can be implicitly casted to nullptr_t
+ explicit Optional(int val) = delete;
+ Optional<T>& operator=(int val) = delete;
+ /*!
+ * \return A not-null container value in the optional.
+ * \note This function performs not-null checking.
+ */
+ T value() const {
+ CHECK(data_ != nullptr);
+ return T(data_);
+ }
+ /*!
+ * \return The contained value if the Optional is not null
+ * otherwise return the default_value.
+ */
+ T value_or(T default_value) const {
Review comment:
It depends on whether we take the default or non-default path. If we take the default pass, passing by value is better than pass by argument. Perhaps we can add an overload to both cases later.
If the user passes in an rvalue, then it is always batter to pass by value.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services