gemini-code-assist[bot] commented on code in PR #224:
URL: https://github.com/apache/tvm-ffi/pull/224#discussion_r2491873352
##########
src/ffi/extra/env_context.cc:
##########
@@ -139,8 +139,10 @@ int TVMFFIEnvTensorAlloc(DLTensor* prototype,
TVMFFIObjectHandle* out) {
}
DLManagedTensorVersioned* dlpack_tensor = nullptr;
int ret = (*dlpack_alloc)(prototype, &dlpack_tensor, nullptr,
TVMFFIEnvTensorAllocSetError);
- TVM_FFI_ICHECK(dlpack_tensor != nullptr);
if (ret != 0) return ret;
+ // need to first check ret so we can properly propagate the error from
caller side.
+ TVM_FFI_CHECK(dlpack_tensor != nullptr, RuntimeError)
+ << "TVMFFIEnvTensorAlloc: failed to a tensor from the allocator";
Review Comment:
There's a minor typo in this error message. It currently reads "failed to a
tensor", which could be clearer. I suggest changing it to "failed to allocate a
tensor" for better readability.
```suggestion
TVM_FFI_CHECK(dlpack_tensor != nullptr, RuntimeError)
<< "TVMFFIEnvTensorAlloc: failed to allocate a tensor from the
allocator";
```
##########
tests/cpp/extra/test_c_env_api.cc:
##########
@@ -74,12 +74,13 @@ TEST(CEnvAPI, TVMFFIEnvTensorAlloc) {
TEST(CEnvAPI, TVMFFIEnvTensorAllocError) {
auto old_allocator = TVMFFIEnvGetDLPackManagedTensorAllocator();
TVMFFIEnvSetDLPackManagedTensorAllocator(TestDLPackManagedTensorAllocatorError,
0, nullptr);
- EXPECT_THROW(
- {
- Tensor::FromEnvAlloc(TVMFFIEnvTensorAlloc, {1, 2, 3},
DLDataType({kDLFloat, 32, 1}),
- DLDevice({kDLCPU, 0}));
- },
- tvm::ffi::Error);
+ try {
+ Tensor::FromEnvAlloc(TVMFFIEnvTensorAlloc, {1, 2, 3},
DLDataType({kDLFloat, 32, 1}),
+ DLDevice({kDLCPU, 0}));
+ } catch (const tvm::ffi::Error& e) {
+ EXPECT_EQ(e.kind(), "RuntimeError");
+ EXPECT_EQ(e.message(), "TestDLPackManagedTensorAllocatorError");
+ }
Review Comment:
The test now correctly checks the exception's details. However, if
`Tensor::FromEnvAlloc` were to stop throwing an exception in the future, this
test would pass silently, potentially masking a regression. To make the test
more robust, it's a good practice to add a `FAIL()` call after the function
that is expected to throw. This ensures the test fails if no exception is
caught.
```suggestion
try {
Tensor::FromEnvAlloc(TVMFFIEnvTensorAlloc, {1, 2, 3},
DLDataType({kDLFloat, 32, 1}),
DLDevice({kDLCPU, 0}));
FAIL() << "Expected an exception of type tvm::ffi::Error to be thrown.";
} catch (const tvm::ffi::Error& e) {
EXPECT_EQ(e.kind(), "RuntimeError");
EXPECT_EQ(e.message(), "TestDLPackManagedTensorAllocatorError");
}
```
--
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]
