gemini-code-assist[bot] commented on code in PR #664:
URL: https://github.com/apache/tvm-ffi/pull/664#discussion_r3574230674
##########
include/tvm/ffi/enum.h:
##########
@@ -126,32 +115,80 @@ class Enum : public ObjectRef {
/// \endcond
};
-template <typename EnumClsObj>
-inline Enum EnumObj::Get(const String& name) {
- static_assert(std::is_base_of_v<EnumObj, EnumClsObj>,
- "EnumObj::Get<T> requires T to be a subclass of EnumObj");
- const TVMFFITypeAttrColumn* column = GetEnumEntriesColumn();
- int32_t type_index = EnumClsObj::_GetOrAllocRuntimeTypeIndex();
- if (column != nullptr) {
- int32_t offset = type_index - column->begin_index;
- if (offset >= 0 && offset < column->size) {
- const TVMFFIAny* stored = &column->data[offset];
- if (stored->type_index != kTVMFFINone) {
- Dict<String, ObjectRef> entries =
- AnyView::CopyFromTVMFFIAny(*stored).cast<Dict<String,
ObjectRef>>();
- auto it = entries.find(name);
- if (it != entries.end()) {
- return details::ObjectUnsafe::ObjectRefFromObjectPtr<Enum>(
-
details::ObjectUnsafe::ObjectPtrFromObjectRef<EnumObj>((*it).second));
- }
- }
- }
+/*!
+ * \brief Base object for payload enums whose public value is an integer.
+ */
+class IntEnumObj : public EnumObj {
+ public:
+ /// \cond Doxygen_Suppress
+ TVM_FFI_DECLARE_OBJECT_INFO("ffi.IntEnum", IntEnumObj, EnumObj);
+ /// \endcond
+};
+
+/*!
+ * \brief ObjectRef wrapper for ``IntEnumObj``.
+ */
+class IntEnum : public Enum {
+ public:
+ /// \cond Doxygen_Suppress
+ TVM_FFI_DEFINE_OBJECT_REF_METHODS_NULLABLE(IntEnum, Enum, IntEnumObj);
+ /// \endcond
+};
+
+/*!
+ * \brief Base object for payload enums whose public value is a string.
+ */
+class StrEnumObj : public EnumObj {
+ public:
+ /// \cond Doxygen_Suppress
+ TVM_FFI_DECLARE_OBJECT_INFO("ffi.StrEnum", StrEnumObj, EnumObj);
+ /// \endcond
+};
+
+/*!
+ * \brief ObjectRef wrapper for ``StrEnumObj``.
+ */
+class StrEnum : public Enum {
+ public:
+ /// \cond Doxygen_Suppress
+ TVM_FFI_DEFINE_OBJECT_REF_METHODS_NULLABLE(StrEnum, Enum, StrEnumObj);
+ /// \endcond
+};
+
+template <typename Index>
+inline Enum EnumObj::GetByIndex(int32_t type_index, const Index& index) {
+ static reflection::TypeAttrColumn
state_column(reflection::type_attr::kEnumState);
+ if (AnyView value = state_column[type_index]; value != nullptr) {
+ EnumState state = value.cast<EnumState>();
+ if (auto entry = state->indexes.Get(Any(index))) return
entry->as_or_throw<Enum>();
Review Comment:
The `entry` variable is of type `Optional<ObjectRef>`, so `entry->`
dereferences to `ObjectRef*`. Since `ObjectRef` does not have an `as_or_throw`
member function (which is typically defined on `Any` and `AnyView`), this will
cause a compilation error. Instead, you should use `downcast<Enum>(*entry)` to
cast the `ObjectRef` to `Enum`.
```suggestion
if (auto entry = state->indexes.Get(Any(index))) return
downcast<Enum>(*entry);
```
##########
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:
Removing `except *` from the `_dispatch_fn_t` typedef and the converter
functions (like `_tc_convert_any`, `_tc_convert_int`, etc.) is extremely
dangerous. Since these functions raise Python exceptions (such as
`_ConvertError`), omitting the exception propagation specifier will cause
Cython to ignore the exceptions at the call site. The exception will remain set
in the Python thread state and \"leak\" out, being raised unexpectedly at some
unrelated later statement, or causing silent failures and undefined behavior.
Please restore `except *` to ensure proper exception propagation.
```
ctypedef CAny (*_dispatch_fn_t)(_TypeConverter, object, bint*) except *
```
##########
src/ffi/extra/dataclass.cc:
##########
@@ -1925,8 +1978,21 @@ void BindFieldArgs(Object* obj, const AutoInitInfo&
info, const TVMFFIAny* raw_a
std::vector<bool> field_set(info.all_fields.size(), false);
auto set_field = [&](size_t fi, const TVMFFIAny* value) {
- void* addr = reinterpret_cast<char*>(obj) +
info.all_fields[fi].info->offset;
- TVM_FFI_CHECK_SAFE_CALL(refl::CallFieldSetter(info.all_fields[fi].info,
addr, value));
+ const TVMFFIFieldInfo* field_info = info.all_fields[fi].info;
+ void* addr = reinterpret_cast<char*>(obj) + field_info->offset;
+ int ret_code = refl::CallFieldSetter(field_info, addr, value);
+ if (ret_code != 0) {
+ Error err = details::MoveFromSafeCallRaised();
+ auto field_name = std::string_view(field_info->name.data,
field_info->name.size);
+ std::string message;
+ message.reserve(info.type_key.size() + field_name.size() +
err.message().size() + 32);
+ message.append(info.type_key);
+ message.append(".__ffi_init__() field '");
+ message.append(field_name);
+ message.append("'");
+ AppendNestedErrorMessage(&message, err.message());
+ throw Error(err.kind(), std::move(message), err.backtrace(), err,
std::nullopt);
+ }
Review Comment:
If `refl::CallFieldSetter` returns a non-zero code but the thread-local
error is not set or has been cleared, `details::MoveFromSafeCallRaised()` may
return a null/undefined `Error` object. Calling `err.message()`, `err.kind()`,
or `err.backtrace()` on an undefined `Error` will result in a null-pointer
dereference and a segmentation fault. Please add a check to ensure
`err.defined()` is true, and fall back to a default error message if it is not.
```c
if (ret_code != 0) {\n Error err =
details::MoveFromSafeCallRaised();\n auto field_name =
std::string_view(field_info->name.data, field_info->name.size);\n
std::string message;\n message.append(info.type_key);\n
message.append(\".__ffi_init__() field '\");\n
message.append(field_name);\n message.append(\"'\");\n if
(err.defined()) {\n message.reserve(info.type_key.size() +
field_name.size() + err.message().size() + 32);\n
AppendNestedErrorMessage(&message, err.message());\n throw
Error(err.kind(), std::move(message), err.backtrace(), err, std::nullopt);\n
} else {\n message.append(\": unknown error during field
assignment\");\n throw Error(\"ValueError\", std::move(message), \"\",
Error(), std::nullopt);\n }\n }
```
##########
python/tvm_ffi/cython/pyclass_type_converter.pxi:
##########
@@ -82,11 +82,16 @@ class _ConvertError(Exception):
return self.args[0]
+cdef str _tc_indent_message(object message, str prefix):
+ cdef str text = str(message)
+ return prefix + text.replace("\n", "\n" + prefix)
+
+
# ---------------------------------------------------------------------------
# Converters (1/N): Simple value converters
# ---------------------------------------------------------------------------
-cdef CAny _tc_convert_any(_TypeConverter _conv, object value, bint* changed)
except *:
+cdef CAny _tc_convert_any(_TypeConverter _conv, object value, bint* changed):
Review Comment:
Please restore `except *` to this and all other converter functions in this
file. Without `except *`, any raised `_ConvertError` will not be propagated
correctly by Cython, leading to leaked exceptions and undefined behavior.
```
cdef CAny _tc_convert_any(_TypeConverter _conv, object value, bint* changed)
except *:
```
##########
include/tvm/ffi/type_traits.h:
##########
@@ -536,6 +537,88 @@ struct TypeTraits<void*> : public TypeTraitsBase {
}
};
+namespace details {
+
+TVM_FFI_INLINE static std::optional<std::string_view> TryGetStringViewFromAny(
+ const TVMFFIAny* src) {
+ if (src->type_index == TypeIndex::kTVMFFIRawStr) {
+ return std::string_view(src->v_c_str);
+ }
+ if (src->type_index == TypeIndex::kTVMFFISmallStr) {
+ TVMFFIByteArray bytes = TVMFFISmallBytesGetContentByteArray(src);
+ return std::string_view(bytes.data, bytes.size);
+ }
+ if (src->type_index == TypeIndex::kTVMFFIStr) {
+ TVMFFIByteArray* bytes = TVMFFIBytesGetByteArrayPtr(src->v_obj);
+ return std::string_view(bytes->data, bytes->size);
+ }
+ return std::nullopt;
+}
+
+TVM_FFI_INLINE static std::optional<DLDeviceType>
TryParseDLDeviceType(std::string_view name) {
+ if (name == "llvm" || name == "cpu" || name == "c" || name == "test") return
kDLCPU;
+ if (name == "cuda" || name == "nvptx") return kDLCUDA;
+ if (name == "cl" || name == "opencl") return kDLOpenCL;
+ if (name == "vulkan") return kDLVulkan;
+ if (name == "metal") return kDLMetal;
+ if (name == "vpi") return kDLVPI;
+ if (name == "rocm") return kDLROCM;
+ if (name == "ext_dev") return kDLExtDev;
+ if (name == "hexagon") return kDLHexagon;
+ if (name == "webgpu") return kDLWebGPU;
+ if (name == "maia") return kDLMAIA;
+ if (name == "trn") return kDLTrn;
+ return std::nullopt;
+}
+
+TVM_FFI_INLINE static std::optional<int32_t>
TryParseDLDeviceIndex(std::string_view index) {
+ if (index.empty()) return std::nullopt;
+ int64_t sign = 1;
+ size_t pos = 0;
+ if (index[0] == '-') {
+ sign = -1;
+ pos = 1;
+ }
+ if (pos >= index.size()) return std::nullopt;
+ int64_t value = 0;
+ for (; pos < index.size(); ++pos) {
+ char ch = index[pos];
+ if (ch < '0' || ch > '9') return std::nullopt;
+ value = value * 10 + (ch - '0');
+ if (value > std::numeric_limits<int32_t>::max()) return std::nullopt;
Review Comment:
The check `if (value > std::numeric_limits<int32_t>::max())` inside the loop
will prematurely reject the valid `int32_t` minimum value `-2147483648` (since
`2147483648 > 2147483647`). To support the full range of `int32_t` while still
preventing `int64_t` overflow during parsing, you can change the threshold
inside the loop to `2147483648LL`.
```c
if (value > 2147483648LL) return std::nullopt;
```
##########
python/tvm_ffi/utils/descriptors.py:
##########
@@ -0,0 +1,61 @@
+# 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
+#
+# http://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.
+"""Descriptor utilities for the tvm_ffi Python package."""
+
+from __future__ import annotations
+
+from typing import Any, Callable, Generic, TypeVar, overload
+
+_T = TypeVar("_T")
+
+
+class init_property(Generic[_T]):
+ """Auto-registered C++ field with eager computation at ``__init__`` time.
+
+ ``@py_class`` detects ``init_property`` descriptors and registers each one
+ as ``field(init=False, structural_eq="ignore")``, so the computed value
+ lives in C++ object storage and is accessible cross-language. The value
+ is computed once — immediately after ``__ffi_init__`` — and stored via the
+ field's C++ slot. Subsequent reads go directly to C++ memory.
+
+ The return annotation of the decorated function is injected into the class
+ ``__annotations__`` during class body execution so the field resolution
+ machinery picks it up automatically. If no return annotation is present,
+ ``typing.Any`` is used.
+ """
+
+ def __init__(self, func: Callable[[Any], _T]) -> None:
+ self.func = func
+ self.name: str | None = None
+ self._return_annotation: Any = func.__annotations__.get("return")
+
+ @overload
+ def __get__(self, obj: None, objtype: type) -> init_property[_T]: ...
+ @overload
+ def __get__(self, obj: object, objtype: type) -> _T: ...
+ def __get__(self, obj: object | None, objtype: type) -> _T |
init_property[_T]:
+ return self
+
+ def __set_name__(self, owner: type, name: str) -> None:
+ self.name = name
+
+ ann = self._return_annotation if self._return_annotation is not None
else Any
+ # Inject into the owner's own __annotations__ so on_fields_resolved
+ # processes this name as a typed field.
+ if "__annotations__" not in owner.__dict__:
+ owner.__annotations__ = {}
+ owner.__annotations__[name] = ann
Review Comment:
In Python 3.14+ (PEP 749), `__annotations__` is a descriptor and is not
stored in `owner.__dict__` by default. Checking `\"__annotations__\" not in
owner.__dict__` and then overwriting it with `{}` will completely erase any
existing lazy annotations on the class. To safely preserve existing annotations
across all Python versions, you should retrieve the existing annotations using
`getattr(owner, \"__annotations__\", None)` and copy them into the new
dictionary.
```python
if \"__annotations__\" not in owner.__dict__:\n existing
= getattr(owner, \"__annotations__\", None)\n owner.__annotations__
= dict(existing) if existing else {}\n owner.__annotations__[name] = ann
```
--
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]
