Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

GH-44464: [C++] Added rvalue-reference-qualified overload for arrow::Result::status() returning value instead of reference #44477

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

GH-44464: [C++] Added rvalue-reference-qualified overload for arrow::Result::status() returning value instead of reference #44477

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions cpp/src/arrow/result.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_; }
Copy link
Member

@bkietz bkietz Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Status status() && { return status_; }
Status status() && { return ok() ? Status::OK() : std::move(status_); }

Without the move in here, we wind up copying status out of this and into the return value. See also absl::StatusOr<T>::status()&&:

https://github.com/abseil/abseil-cpp/blob/master/absl/status/statusor.h#L703

Copy link
Member

@bkietz bkietz Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, this requires default construction of the value in the result or constructing an error status placeholder. Maybe not worth it, but we should add a comment here since others will also assume this is an error

Copy link
Member

@bkietz bkietz Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened #44491 to suggest how we could make it cheaper to put a placeholder error status in here

draft PR #44493

Copy link
Author

@igor-anferov igor-anferov Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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…

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bkietz I'd love to hear your feedback on that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue with this is that std::move(status_) will leave status_ in an OK state

Please mark the result as an error with a placeholder status and move the error status out. This won't be any more expensive than what you've already written, and we can replace the placeholder with a static status after #44493

Suggested change
Status status() && { return status_; }
Status status() && {
if (ok()) return Status::OK();
auto out = std::move(status_);
status_ = Status::UnknownError("Uninitialized Result<T>");
return out;
}

There is a proposal for this, which will likely make it into C++26

Whenever this is ready, it'll make constructs like static error statuses much nicer.


/// Gets the stored `T` value.
///
Expand Down Expand Up @@ -350,7 +356,7 @@ class [[nodiscard]] Result : public util::EqualityComparable<Result<T>> {
std::is_constructible<U, T>::value>::type>
Status Value(U* out) && {
if (!ok()) {
return status();
return std::move(*this).status();
}
*out = U(MoveValueUnsafe());
return Status::OK();
Expand Down Expand Up @@ -380,7 +386,7 @@ class [[nodiscard]] Result : public util::EqualityComparable<Result<T>> {
typename EnsureResult<decltype(std::declval<M&&>()(std::declval<T&&>()))>::type Map(
M&& m) && {
if (!ok()) {
return status();
return std::move(*this).status();
}
return std::forward<M>(m)(MoveValueUnsafe());
}
Expand All @@ -402,7 +408,7 @@ class [[nodiscard]] Result : public util::EqualityComparable<Result<T>> {
std::is_constructible<U, T>::value>::type>
Result<U> As() && {
if (!ok()) {
return status();
return std::move(*this).status();
}
return U(MoveValueUnsafe());
}
Expand Down
Loading