-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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.
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
GH-44464: [C++] Added rvalue-reference-qualified overload for arrow::Result::status() returning value instead of reference #44477
base: main
Are you sure you want to change the base?
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM since seems that abseil do the same. Also paper like https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2718r0.html might prevent from this behavior...
…rrow::Result::status() returning value instead of reference
2449343
to
5b5e898
Compare
Will merge this if no negative comment tonight |
Rationale for this change
In the current implementation,
arrow::Result::status()
always returns the internalstatus_
field by a const lvalue reference, regardless of the value category ofResult
. This can lead to potential bugs. For example, consider the following code:In this case, the call to
status.ok()
results in undefined behavior becausestatus
is a dangling const lvalue reference that points to an object returned byfunctionReturningArrowResult()
, which is destroyed after the semicolon.If
arrow::Result
had two overloads of thestatus()
method for different reference qualifiers:This would prevent such bugs and potentially allow for better optimization, as the
Status
could be moved from an expiringResult
object.What changes are included in this PR?
This PR adds the proposed overload for the
arrow::Result::status()
method and makes other rvalue-qualifiedarrow::Result
methods preserve object ref-category during tailstatus()
calls.Unfortunately, we can't move the
status_
field in the rvalue-qualifiedstatus()
method, as the state ofstatus_
must be preserved until the destructor is called. This is because thestorage_
field is either destructed or considered empty based on the state ofstatus_
.Are these changes tested?
Since this change is trivial (the new overload doesn't modify the
Result
object and returnsStatus
by value), there's nothing significant to test, so no new tests were added.Are there any user-facing changes?
No existing code will be broken by this change. In all cases where
status()
is called on an lvalueResult
, the same reference-returning overload will be called. Meanwhile, code callingstatus()
on an rvalueResult
will invoke the new overload, returningStatus
by value instead.