junrushao commented on code in PR #666:
URL: https://github.com/apache/tvm-ffi/pull/666#discussion_r3583240909
##########
python/tvm_ffi/cython/pyclass_type_converter.pxi:
##########
@@ -36,9 +36,9 @@ cdef object _INT64_MIN = -(1 << 63)
cdef object _INT64_MAX = (1 << 63) - 1
cdef int _VALUE_PROTOCOL_MAX_DEPTH = 64
cdef str _TYPE_ATTR_FFI_CONVERT = "__ffi_convert__"
-cdef str _TYPE_ATTR_ENUM_VALUE_ENTRIES = "__ffi_enum_value_entries__"
+cdef str _TYPE_ATTR_ENUM_STATE = "__ffi_enum__"
cdef class _TypeConverter
-ctypedef CAny (*_dispatch_fn_t)(_TypeConverter, object, bint*) except *
+ctypedef CAny (*_dispatch_fn_t)(_TypeConverter, object, bint*)
Review Comment:
Thanks for flagging this. This thread is outdated after the PR was narrowed
and rebased: `python/tvm_ffi/cython/pyclass_type_converter.pxi` is no longer
changed by PR #666 relative to `main`. I’ve therefore left the converter
exception specifications unchanged in this PR so the concern can be handled in
the change that owns that file.
##########
include/tvm/ffi/container/variant.h:
##########
@@ -167,7 +185,8 @@ class Variant {
* ObjectRef
*/
TVM_FFI_INLINE Object* GetObjectPtrForHashEqual() const {
- constexpr bool all_object_v = (std::is_base_of_v<ObjectRef, V> && ...);
+ constexpr bool all_object_v =
+ (std::is_base_of_v<ObjectRef, details::object_ptr_type_t<V>> && ...);
static_assert(all_object_v,
"All types used in Variant<...> must be derived from
ObjectRef "
"to enable ObjectPtrHash/ObjectPtrEqual");
Review Comment:
Fixed in `bef03d5`. `GetObjectPtrForHashEqual` now checks each normalized
alternative’s `TypeTraits<...>::field_static_type_index` against
`kTVMFFIObject`, so both `ObjectRef` and `ObjectPtr` alternatives are accepted.
I also added `Variant.ObjectSubclassShorthandHashEqual`; the full C++ test
suite and targeted clang-tidy pass.
##########
include/tvm/ffi/container/variant.h:
##########
@@ -71,6 +71,14 @@ class Variant {
*/
Variant(Variant<V...>&& other) noexcept = default;
+ /*! \brief Convert from a variant whose normalized alternatives are all
contained. */
+ template <typename... U, std::enable_if_t<(variant_contains_v<U> && ...),
int> = 0>
+ Variant(const Variant<U...>& other) : data_(Any(other)) {} // NOLINT(*)
+
+ /*! \brief Move from a variant whose normalized alternatives are all
contained. */
+ template <typename... U, std::enable_if_t<(variant_contains_v<U> && ...),
int> = 0>
+ Variant(Variant<U...>&& other) : data_(Any(std::move(other))) {} //
NOLINT(*)
Review Comment:
Fixed in `bef03d5`. Both converting constructors now require
`!std::is_same_v<Variant, Variant<U...>>`, so exact-type copy and move continue
to use the default constructors. The full C++ test suite and targeted
clang-tidy pass.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
