gemini-code-assist[bot] commented on code in PR #69:
URL: https://github.com/apache/tvm-ffi/pull/69#discussion_r2386387201
##########
include/tvm/ffi/error.h:
##########
@@ -321,6 +321,9 @@ TVM_FFI_CHECK_FUNC(_NE, !=)
#define TVM_FFI_ICHECK(x) \
if (!(x)) TVM_FFI_THROW(InternalError) << "Check failed: (" #x << ") is
false: "
+#define TVM_FFI_CHECK(ErrorKind, cond) \
+ if (!(cond)) TVM_FFI_THROW(ErrorKind) << "Check failed: (" #cond << ") is
false: "
Review Comment:
This macro, like other similar check macros in this file (e.g.,
`TVM_FFI_ICHECK`), is susceptible to the 'dangling else' problem. For example:
```cpp
if (condition)
TVM_FFI_CHECK(ValueError, false);
else
do_something();
```
This will be expanded to:
```cpp
if (condition)
if (!(false)) TVM_FFI_THROW(ValueError) << ...;
else
do_something();
```
Here, the `else` will bind to the inner `if`, not the outer one as intended.
This is a logical error because `do_something()` will be executed when
`condition` is true and the check inside the macro fails, instead of when
`condition` is false.
A common way to fix this is to use a `do-while(0)` loop. However, this would
break the ability to stream additional messages to the macro (e.g.,
`TVM_FFI_CHECK(...) << "more info";`), which is a pattern used in the tests.
To make it safe against dangling else, you could change the definition to:
```cpp
#define TVM_FFI_CHECK(ErrorKind, cond) \
do { \
if (!(cond)) { \
TVM_FFI_THROW(ErrorKind) << "Check failed: (" #cond << ") is false: ";
\
} \
} while (0)
```
This would require changing call sites to not use the streaming operator on
the macro itself. Given this is a pre-existing pattern in the codebase, a wider
discussion might be needed on how to handle this. For this new macro, I'd
recommend considering a safer implementation even if it means adjusting its
usage.
--
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]
