gemini-code-assist[bot] commented on code in PR #666:
URL: https://github.com/apache/tvm-ffi/pull/666#discussion_r3583083833
##########
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:
To prevent the template constructors from matching when `Variant<U...>` is
exactly the same type as `Variant<V...>`, we should add a constraint
`!std::is_same_v<Variant, Variant<U...>>` to the `std::enable_if_t` condition.
This avoids potential overload resolution issues, ambiguity, or selecting the
slow template path (which constructs an `Any` and copies/moves) instead of the
default copy/move constructors (which directly copy/move the underlying `Any`
data).
```c
/*! \brief Convert from a variant whose normalized alternatives are all
contained. */
template <typename... U, std::enable_if_t<!std::is_same_v<Variant,
Variant<U...>> &&
(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<!std::is_same_v<Variant,
Variant<U...>> &&
(variant_contains_v<U> &&
...),
int> = 0>
Variant(Variant<U...>&& other) : data_(Any(std::move(other))) {} //
NOLINT(*)
```
##########
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:
With the introduction of `ObjectPtr` support as shorthand normalized
alternatives in `Variant`, the alternatives can now be `ObjectPtr<T>` (which do
not inherit from `ObjectRef`). As a result, `std::is_base_of_v<ObjectRef,
details::object_ptr_type_t<V>>` will evaluate to `false` for these
alternatives, causing the `static_assert` to fail and preventing `Variant`
types containing normalized `ObjectPtr` alternatives from being used with
`ObjectPtrHash` or `ObjectPtrEqual` (e.g., as keys in `Map` or `Set`).
We should update the check to allow any type `T` where
`TypeTraits<T>::field_static_type_index == TypeIndex::kTVMFFIObject`, which
correctly identifies both `ObjectRef` subclasses and `ObjectPtr` types.
```c
constexpr bool all_object_v =
(TypeTraits<details::object_ptr_type_t<V>>::field_static_type_index
==
TypeIndex::kTVMFFIObject &&
...);
static_assert(all_object_v,
"All types used in Variant<...> must be derived from
ObjectRef "
"or ObjectPtr to enable ObjectPtrHash/ObjectPtrEqual");
```
##########
include/tvm/ffi/container/variant.h:
##########
@@ -83,6 +91,16 @@ class Variant {
*/
Variant& operator=(Variant<V...>&& other) noexcept = default;
+ template <typename... U, std::enable_if_t<(variant_contains_v<U> && ...),
int> = 0>
+ TVM_FFI_INLINE Variant& operator=(const Variant<U...>& other) {
+ return operator=(Variant(other));
+ }
+
+ template <typename... U, std::enable_if_t<(variant_contains_v<U> && ...),
int> = 0>
+ TVM_FFI_INLINE Variant& operator=(Variant<U...>&& other) {
+ return operator=(Variant(std::move(other)));
+ }
Review Comment:
Similarly to the constructors, the template assignment operators should be
constrained with `!std::is_same_v<Variant, Variant<U...>>` to prevent them from
matching when `Variant<U...>` is the same type as `Variant<V...>`, ensuring the
default copy/move assignment operators are preferred and avoiding potential
infinite recursion or overload resolution issues.
```c
template <typename... U, std::enable_if_t<!std::is_same_v<Variant,
Variant<U...>> &&
(variant_contains_v<U> &&
...),
int> = 0>
TVM_FFI_INLINE Variant& operator=(const Variant<U...>& other) {
return operator=(Variant(other));
}
template <typename... U, std::enable_if_t<!std::is_same_v<Variant,
Variant<U...>> &&
(variant_contains_v<U> &&
...),
int> = 0>
TVM_FFI_INLINE Variant& operator=(Variant<U...>&& other) {
return operator=(Variant(std::move(other)));
}
```
--
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]
