gemini-code-assist[bot] commented on code in PR #228:
URL: https://github.com/apache/tvm-ffi/pull/228#discussion_r2493765581
##########
include/tvm/ffi/function_details.h:
##########
@@ -182,11 +189,74 @@ class ArgValueWithContext {
FGetFuncSignature f_sig_;
};
+template <typename Type>
+class ArgValueWithContext<Type, /*NeedStorage=*/true> {
+ public:
+ // We only support pure value type to convert to, to avoid any dangling
reference issues.
+ // C++ will automatically eliminate unnecessary constructor calls by NRVO,
+ // so return by value won't cause performance issue.
+ static_assert(std::is_same_v<Type,
std::remove_const_t<std::remove_reference_t<Type>>>);
+ /*!
+ * \brief move constructor from another return value.
+ * \param args The argument list
+ * \param arg_index In a function call, this argument is at index arg_index
(0-indexed).
+ * \param optional_name Name of the function being called. Can be nullptr if
the function is
+ * not.
+ * \param f_sig Pointer to static function outputting signature of the
function being called.
+ * named.
+ */
+ TVM_FFI_INLINE ArgValueWithContext(const AnyView* args, int32_t arg_index,
+ const std::string* optional_name,
FGetFuncSignature f_sig)
+ : args_(args), arg_index_(arg_index), optional_name_(optional_name),
f_sig_(f_sig) {}
+
+ // to avoid dangling reference, we only allow one-time conversion to Type&
+ TVM_FFI_INLINE operator Type&() { // NOLINT(google-explicit-constructor)
+ TVM_FFI_ICHECK(!temp_storage_.has_value())
+ << "ArgValueWithContext<Type> can only be converted to Type& once.";
+ if constexpr (std::is_same_v<Type, AnyView>) {
+ temp_storage_.emplace(args_[arg_index_]);
+ } else if constexpr (std::is_same_v<Type, Any>) {
+ temp_storage_.emplace(Any(args_[arg_index_]));
+ } else {
+ temp_storage_ = args_[arg_index_].try_cast<Type>();
+ if (!temp_storage_.has_value()) {
+ TVMFFIAny any_data = args_[arg_index_].CopyToTVMFFIAny();
+ TVM_FFI_THROW(TypeError) << "Mismatched type on argument #" <<
arg_index_
+ << " when calling: `"
+ << (optional_name_ == nullptr ? "" :
*optional_name_)
+ << (f_sig_ == nullptr ? "" : (*f_sig_)()) <<
"`. Expected `"
+ << Type2Str<Type>::v() << "` but got `"
+ <<
TypeTraits<Type>::GetMismatchTypeInfo(&any_data) << '`';
+ }
+ }
+ return temp_storage_.value();
+ }
+
+ private:
+ const AnyView* args_;
+ int32_t arg_index_;
+ const std::string* optional_name_;
+ FGetFuncSignature f_sig_;
+ std::optional<Type> temp_storage_;
+};
+
+/**
+ * \brief Auxilary argument value with context for error reporting
+ * For `Type &` non-const lvalue reference types, we need to
+ * store a temporary value to avoid dangling reference issues.
+ * For `const Type`, `Type &&`, returning value can be cast to
+ * the desired type directly.
+ */
+template <typename Type>
+using ArgValue =
ArgValueWithContext<std::remove_const_t<std::remove_reference_t<Type>>,
+ (!std::is_const_v<Type> &&
std::is_lvalue_reference_v<Type>)>;
Review Comment:
The condition to decide whether `ArgValueWithContext` needs storage is not
optimal for `const` l-value references. Currently, `(!std::is_const_v<Type> &&
std::is_lvalue_reference_v<Type>)` evaluates to `true` for `const T&` types
because `std::is_const_v<const T&>` is `false`. This causes unnecessary storage
allocation and copying for `const` references, which could otherwise safely
bind to temporaries.
The comment at line 245, "For `Type &` non-const lvalue reference types, we
need to store a temporary value", suggests that `const` references should not
require storage.
To fix this and improve performance, the check should be on the underlying
type, not the reference itself. You can use `std::remove_reference_t` to get
the underlying type for the const-check.
```c
using ArgValue =
ArgValueWithContext<std::remove_const_t<std::remove_reference_t<Type>>,
(!std::is_const_v<std::remove_reference_t<Type>> &&
std::is_lvalue_reference_v<Type>)>;
```
##########
include/tvm/ffi/container/stl.h:
##########
@@ -0,0 +1,421 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * htT://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file tvm/ffi/container/stl.h
+ * \brief STL container support.
+ *
+ */
+#ifndef TVM_FFI_CONTAINER_STL_H_
+#define TVM_FFI_CONTAINER_STL_H_
+
+#include <tvm/ffi/base_details.h>
+#include <tvm/ffi/container/array.h>
+#include <tvm/ffi/container/tuple.h>
+#include <tvm/ffi/object.h>
+#include <tvm/ffi/type_traits.h>
+
+#include <algorithm>
+#include <array>
+#include <cstddef>
+#include <optional>
+#include <tuple>
+#include <type_traits>
+#include <variant>
+#include <vector>
+
+#include "tvm/ffi/c_api.h"
+#include "tvm/ffi/error.h"
+
+namespace tvm {
+namespace ffi {
+
+template <typename T, std::size_t Nm>
+struct TypeTraits<std::array<T, Nm>> : public TypeTraitsBase {
+ private:
+ using Self = std::array<T, Nm>;
+ using Array = ::tvm::ffi::Array<T>;
+ static_assert(Nm > 0, "Zero-length std::array is not supported.");
+
+ public:
+ static constexpr int32_t field_static_type_index = TypeIndex::kTVMFFIArray;
+
+ TVM_FFI_INLINE static void CopyToAnyView(const Self& src, TVMFFIAny* result)
{
+ return TypeTraits<Array>::MoveToAny({src.begin(), src.end()}, result);
+ }
+
+ TVM_FFI_INLINE static void MoveToAny(Self&& src, TVMFFIAny* result) {
+ return TypeTraits<Array>::MoveToAny(
+ {std::make_move_iterator(src.begin()),
std::make_move_iterator(src.end())}, result);
+ }
+
+ TVM_FFI_INLINE static bool CheckAnyStrict(const TVMFFIAny* src) {
+ if (src->type_index != TypeIndex::kTVMFFIArray) return false;
+ const ArrayObj& n = *reinterpret_cast<const ArrayObj*>(src->v_obj);
+ // check static length first
+ if (n.size_ != Nm) return false;
+ // then check element type
+ if constexpr (std::is_same_v<T, Any>) {
+ return true;
+ } else {
+ return std::all_of(n.begin(), n.end(),
details::AnyUnsafe::CheckAnyStrict<T>);
+ }
+ }
+
+ TVM_FFI_INLINE static Self CopyFromAnyViewAfterCheck(const TVMFFIAny* src) {
+ auto array = TypeTraits<Array>::CopyFromAnyViewAfterCheck(src);
+ Self result; // no initialization to avoid overhead
+ std::copy_n(std::make_move_iterator(array.begin()), Nm, result.begin());
+ return result;
+ }
+
+ TVM_FFI_INLINE static Self MoveFromAnyAfterCheck(TVMFFIAny* src) {
+ auto array = TypeTraits<Array>::MoveFromAnyAfterCheck(src);
+ Self result; // no initialization to avoid overhead
+ std::copy_n(std::make_move_iterator(array.begin()), Nm, result.begin());
+ return result;
+ }
+
+ TVM_FFI_INLINE static std::optional<Self> TryCastFromAnyView(const
TVMFFIAny* src) {
+ if (!CheckAnyStrict(src)) return std::nullopt;
+ return CopyFromAnyViewAfterCheck(src);
+ }
+
+ TVM_FFI_INLINE static std::string TypeStr() {
+ return "std::array<" + details::Type2Str<T>::v() + ", " +
std::to_string(Nm) + ">";
+ }
+ TVM_FFI_INLINE static std::string TypeSchema() {
+ return R"({"type":"std::array","args":[)" + details::TypeSchema<T>::v() +
"," +
+ std::to_string(Nm) + "]}";
+ }
+};
+
+template <typename T>
+struct TypeTraits<std::vector<T>> : public TypeTraitsBase {
+ private:
+ using Self = std::vector<T>;
+ using Array = ::tvm::ffi::Array<T>;
+
+ public:
+ static constexpr int32_t field_static_type_index = TypeIndex::kTVMFFIArray;
+
+ TVM_FFI_INLINE static void CopyToAnyView(const Self& src, TVMFFIAny* result)
{
+ return TypeTraits<Array>::MoveToAny({src.begin(), src.end()}, result);
+ }
+
+ TVM_FFI_INLINE static void MoveToAny(Self&& src, TVMFFIAny* result) {
+ return TypeTraits<Array>::MoveToAny(
+ {std::make_move_iterator(src.begin()),
std::make_move_iterator(src.end())}, result);
+ }
+
+ TVM_FFI_INLINE static bool CheckAnyStrict(const TVMFFIAny* src) {
+ if (src->type_index != TypeIndex::kTVMFFIArray) return false;
+ const ArrayObj& n = *reinterpret_cast<const ArrayObj*>(src->v_obj);
+ if constexpr (std::is_same_v<T, Any>) {
+ return true;
+ } else {
+ return std::all_of(n.begin(), n.end(),
details::AnyUnsafe::CheckAnyStrict<T>);
+ }
+ }
+
+ TVM_FFI_INLINE static Self CopyFromAnyViewAfterCheck(const TVMFFIAny* src) {
+ auto array = TypeTraits<Array>::CopyFromAnyViewAfterCheck(src);
+ return Self{std::make_move_iterator(array.begin()),
std::make_move_iterator(array.end())};
+ }
+
+ TVM_FFI_INLINE static Self MoveFromAnyAfterCheck(TVMFFIAny* src) {
+ auto array = TypeTraits<Array>::MoveFromAnyAfterCheck(src);
+ return Self{std::make_move_iterator(array.begin()),
std::make_move_iterator(array.end())};
+ }
+
+ TVM_FFI_INLINE static std::optional<Self> TryCastFromAnyView(const
TVMFFIAny* src) {
+ if (!CheckAnyStrict(src)) return std::nullopt;
+ return CopyFromAnyViewAfterCheck(src);
+ }
+
+ TVM_FFI_INLINE static std::string TypeStr() {
+ return "std::vector<" + details::Type2Str<T>::v() + ">";
+ }
+ TVM_FFI_INLINE static std::string TypeSchema() {
+ return R"({"type":"std::vector","args":[)" + details::TypeSchema<T>::v() +
"]}";
+ }
+};
+
+template <typename T>
+struct TypeTraits<std::optional<T>> : public TypeTraitsBase {
+ private:
+ using Self = std::optional<T>;
+
+ public:
+ TVM_FFI_INLINE static void CopyToAnyView(const Self& src, TVMFFIAny* result)
{
+ if (src.has_value()) {
+ TypeTraits<Self>::CopyToAnyView(*src, result);
+ } else {
+ TypeTraits<std::nullptr_t>::CopyToAnyView(nullptr, result);
+ }
+ }
+ TVM_FFI_INLINE static void MoveToAny(Self&& src, TVMFFIAny* result) {
+ if (src.has_value()) {
+ TypeTraits<Self>::MoveToAny(std::move(*src), result);
+ } else {
+ TypeTraits<std::nullptr_t>::MoveToAny(nullptr, result);
+ }
+ }
Review Comment:
There's a bug in `CopyToAnyView` and `MoveToAny` for `std::optional<T>`. The
implementation recursively calls `TypeTraits<Self>::...`, where `Self` is
`std::optional<T>`. This will lead to infinite recursion and a stack overflow
if the optional contains a value. The call should be dispatched to
`TypeTraits<T>` for the contained value.
```c
TVM_FFI_INLINE static void CopyToAnyView(const Self& src, TVMFFIAny*
result) {
if (src.has_value()) {
TypeTraits<T>::CopyToAnyView(*src, result);
} else {
TypeTraits<std::nullptr_t>::CopyToAnyView(nullptr, result);
}
}
TVM_FFI_INLINE static void MoveToAny(Self&& src, TVMFFIAny* result) {
if (src.has_value()) {
TypeTraits<T>::MoveToAny(std::move(*src), result);
} else {
TypeTraits<std::nullptr_t>::MoveToAny(nullptr, 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.
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]
