gemini-code-assist[bot] commented on code in PR #78:
URL: https://github.com/apache/tvm-ffi/pull/78#discussion_r2404742920
##########
include/tvm/ffi/optional.h:
##########
@@ -129,13 +129,20 @@ class Optional<T,
std::enable_if_t<!use_ptr_based_optional_v<T> && !std::is_same
* \return the xvalue reference to the stored value.
* \note only use this function after checking has_value()
*/
- TVM_FFI_INLINE T&& operator*() && noexcept { return *std::move(data_); }
+ TVM_FFI_INLINE T&& operator*() && {
+ if (!data_.has_value()) {
+ TVM_FFI_THROW(RuntimeError) << "Back optional access";
+ }
+ return *std::move(data_);
+ }
/*!
* \brief Direct access to the value.
* \return the const reference to the stored value.
* \note only use this function after checking has_value()
*/
- TVM_FFI_INLINE const T& operator*() const& noexcept { return *data_; }
+ TVM_FFI_INLINE const T& operator*() const& {
+ return *data_; // NOLINT(bugprone-unchecked-optional-access)
+ }
Review Comment:
The `noexcept` was removed from the rvalue-qualified `operator*`, and a
check for `has_value()` was added, which is a great improvement for safety.
However, this `const&` overload is now marked with
`NOLINT(bugprone-unchecked-optional-access)` but doesn't perform a check. For
consistency and to prevent potential runtime errors, it would be better to also
add a check here, similar to the rvalue overload.
```c
TVM_FFI_INLINE const T& operator*() const& {
if (!data_.has_value()) {
TVM_FFI_THROW(RuntimeError) << "Back optional access";
}
return *data_;
}
```
--
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]
