gemini-code-assist[bot] commented on code in PR #599:
URL: https://github.com/apache/tvm-ffi/pull/599#discussion_r3318777099
##########
include/tvm/ffi/dtype.h:
##########
@@ -164,6 +164,7 @@ struct TypeTraits<DLDataType> : public TypeTraitsBase {
}
TVM_FFI_INLINE static DLDataType CopyFromAnyViewAfterCheck(const TVMFFIAny*
src) {
+ TVM_FFI_UNSAFE_ASSUME(src->type_index == TypeIndex::kTVMFFIDataType);
return src->v_dtype;
}
Review Comment:
The `TypeTraits<DLDataType>` specialization is missing the
`MoveFromAnyAfterCheck` static method. Since `DLDataType` is a POD type and has
`storage_enabled = true` (inherited from `TypeTraitsBase`), any attempt to
move-cast it from an `Any` (e.g., via `Any::cast<DLDataType>() &&` or
`Expected<DLDataType>::value() &&`) will fail to compile. Adding
`MoveFromAnyAfterCheck` to copy the value (as done for other POD types like
`DLDevice`) resolves this issue.
```c
TVM_FFI_INLINE static DLDataType CopyFromAnyViewAfterCheck(const
TVMFFIAny* src) {
TVM_FFI_UNSAFE_ASSUME(src->type_index == TypeIndex::kTVMFFIDataType);
return src->v_dtype;
}
TVM_FFI_INLINE static DLDataType MoveFromAnyAfterCheck(TVMFFIAny* src) {
// POD type, we can just copy the value
return CopyFromAnyViewAfterCheck(src);
}
```
##########
include/tvm/ffi/expected.h:
##########
@@ -161,14 +201,14 @@ class Expected {
*/
template <typename U = std::remove_cv_t<T>>
TVM_FFI_INLINE T value_or(U&& default_value) const {
- if (is_ok()) {
- return data_.cast<T>();
+ if (TVM_FFI_PREDICT_TRUE(is_ok())) {
+ return details::AnyUnsafe::CopyFromAnyViewAfterCheck<T>(data_);
}
return T(std::forward<U>(default_value));
}
Review Comment:
To fully support the move-optimization goals of this PR, `Expected<T>`
should provide an rvalue overload for `value_or`. Currently, calling `value_or`
on an rvalue `Expected<T>` (e.g., `std::move(expected).value_or(default_val)`)
binds to the `const` lvalue overload, resulting in an unnecessary copy of the
contained value. Splitting `value_or` into `const&` and `&&` overloads allows
moving the value out of the `Expected` container when it is a temporary.
```c
template <typename U = std::remove_cv_t<T>>
TVM_FFI_INLINE T value_or(U&& default_value) const& {
if (TVM_FFI_PREDICT_TRUE(is_ok())) {
return details::AnyUnsafe::CopyFromAnyViewAfterCheck<T>(data_);
}
return T(std::forward<U>(default_value));
}
template <typename U = std::remove_cv_t<T>>
TVM_FFI_INLINE T value_or(U&& default_value) && {
if (TVM_FFI_PREDICT_TRUE(is_ok())) {
return details::AnyUnsafe::MoveFromAnyAfterCheck<T>(std::move(data_));
}
return T(std::forward<U>(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.
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]
