igor-anferov commented on code in PR #44477:
URL: https://github.com/apache/arrow/pull/44477#discussion_r1809574569
##########
cpp/src/arrow/result.h:
##########
@@ -294,7 +294,13 @@ class [[nodiscard]] Result : public
util::EqualityComparable<Result<T>> {
///
/// \return The stored non-OK status object, or an OK status if this object
/// has a value.
- constexpr const Status& status() const { return status_; }
+ constexpr const Status& status() const& { return status_; }
+
+ /// Gets the stored status object, or an OK status if a `T` value is stored.
+ ///
+ /// \return The stored non-OK status object, or an OK status if this object
+ /// has a value.
+ Status status() && { return status_; }
Review Comment:
The issue with this is that `std::move(status_)` will leave `status_` in an
`OK` state (since the internal `state_` pointer will be `NULL` afterward). As a
result, the Result destructor will attempt to call a destructor on the
`storage_` field, leading to undefined behavior because `storage_` is
uninitialized when the original `Result` was constructed in an error state. One
possible solution (assuming we need to minimize the size of `Result` without
simply putting `status_` and `storage_` in a `variant`) is for the move
constructor of `Status` to preserve the binary OK/error state of the moved-out
object. This could be achieved without changing the size of `Status` by using
pointer tagging for its `state_` field. There is a
[proposal](https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2024/p3125r1.html)
for this, which will likely make it into C++26, but until then, there’s no
truly safe way to do this while complying with the current C++ standard…
--
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]
