gemini-code-assist[bot] commented on code in PR #396:
URL: https://github.com/apache/tvm-ffi/pull/396#discussion_r2678899086
##########
src/ffi/error.cc:
##########
@@ -111,3 +111,30 @@ int TVMFFIErrorCreate(const TVMFFIByteArray* kind, const
TVMFFIByteArray* messag
}
TVM_FFI_LOG_EXCEPTION_CALL_END(TVMFFIErrorCreate);
}
+
+int TVMFFIErrorCreateWithCauseAndExtraContext(
+ const TVMFFIByteArray* kind, const TVMFFIByteArray* message, const
TVMFFIByteArray* backtrace,
+ TVMFFIObjectHandle cause_chain, TVMFFIObjectHandle extra_context,
TVMFFIObjectHandle* out) {
+ // log other errors to the logger
+ TVM_FFI_LOG_EXCEPTION_CALL_BEGIN();
+ try {
+ tvm::ffi::Error cause_chain_error =
+
tvm::ffi::details::ObjectUnsafe::ObjectRefFromObjectPtr<tvm::ffi::Error>(
+
tvm::ffi::details::ObjectUnsafe::ObjectPtrFromUnowned<tvm::ffi::ErrorObj>(
+ static_cast<tvm::ffi::ErrorObj*>(cause_chain)));
+ tvm::ffi::ObjectRef extra_context_ref =
+
tvm::ffi::details::ObjectUnsafe::ObjectRefFromObjectPtr<tvm::ffi::ObjectRef>(
+
tvm::ffi::details::ObjectUnsafe::ObjectPtrFromUnowned<tvm::ffi::Object>(
+ static_cast<tvm::ffi::Object*>(extra_context)));
+
+ tvm::ffi::Error error(std::string(kind->data, kind->size),
+ std::string(message->data, message->size),
+ std::string(backtrace->data, backtrace->size),
+ std::move(cause_chain_error),
std::move(extra_context_ref));
Review Comment:
The current implementation has two issues:
1. **Unsafe Type Cast**: It performs a `static_cast` from
`TVMFFIObjectHandle` (`void*`) directly to `tvm::ffi::ErrorObj*` for
`cause_chain`. This is unsafe and can lead to undefined behavior if the handle
is not of the expected type. It's much safer to cast to the base
`tvm::ffi::Object*` first and then perform a runtime type check using
`IsInstance`.
2. **Unclear `nullptr` Handling**: The handling of `nullptr` for
`cause_chain` and `extra_context` is confusing. It results in an
`std::optional` containing a null `ObjectRef` rather than an empty
`std::optional`. The suggested change makes this more idiomatic by creating an
empty optional when the handle is `nullptr`.
The following suggestion addresses both points, making the code safer and
more readable.
```c
std::optional<tvm::ffi::Error> cause_chain_error;
if (cause_chain) {
auto* obj = static_cast<tvm::ffi::Object*>(cause_chain);
TVM_FFI_ICHECK(obj->IsInstance<tvm::ffi::ErrorObj>())
<< "TypeError: cause_chain is expected to be an Error object.";
cause_chain_error =
tvm::ffi::details::ObjectUnsafe::ObjectRefFromObjectPtr<tvm::ffi::Error>(
tvm::ffi::details::ObjectUnsafe::ObjectPtrFromUnowned<tvm::ffi::ErrorObj>(obj));
}
std::optional<tvm::ffi::ObjectRef> extra_context_ref;
if (extra_context) {
extra_context_ref =
tvm::ffi::details::ObjectUnsafe::ObjectRefFromObjectPtr<tvm::ffi::ObjectRef>(
tvm::ffi::details::ObjectUnsafe::ObjectPtrFromUnowned<tvm::ffi::Object>(
static_cast<tvm::ffi::Object*>(extra_context)));
}
tvm::ffi::Error error(std::string(kind->data, kind->size),
std::string(message->data, message->size),
std::string(backtrace->data, backtrace->size),
std::move(cause_chain_error),
std::move(extra_context_ref));
```
--
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]
