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.

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

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

Conversation

igor-anferov
Copy link

@igor-anferov igor-anferov commented Oct 19, 2024

Rationale for this change

In the current implementation, arrow::Result::status() always returns the internal status_ field by a const lvalue reference, regardless of the value category of Result. This can lead to potential bugs. For example, consider the following code:

if (auto&& status = functionReturningArrowResult().status(); status.ok())
  return 0;
return -1;

In this case, the call to status.ok() results in undefined behavior because status is a dangling const lvalue reference that points to an object returned by functionReturningArrowResult(), which is destroyed after the semicolon.

If arrow::Result had two overloads of the status() method for different reference qualifiers:

template <…>
class Result {
  …
  auto status() const & -> const Status& { ... }
  auto status() && -> Status { ... }
  …
};

This would prevent such bugs and potentially allow for better optimization, as the Status could be moved from an expiring Result object.

What changes are included in this PR?

This PR adds the proposed overload for the arrow::Result::status() method and makes other rvalue-qualified arrow::Result methods preserve object ref-category during tail status() calls.

Unfortunately, we can't move the status_ field in the rvalue-qualified status() method, as the state of status_ must be preserved until the destructor is called. This is because the storage_ field is either destructed or considered empty based on the state of status_.

Are these changes tested?

Since this change is trivial (the new overload doesn't modify the Result object and returns Status 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 lvalue Result, the same reference-returning overload will be called. Meanwhile, code calling status() on an rvalue Result will invoke the new overload, returning Status by value instead.

Copy link

⚠️ GitHub issue #44464 has been automatically assigned in GitHub to PR creator.

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Oct 21, 2024
Copy link
Member

@mapleFU mapleFU left a 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...

cpp/src/arrow/result.h Outdated Show resolved Hide resolved
@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Oct 21, 2024
…rrow::Result::status() returning value instead of reference
@igor-anferov igor-anferov force-pushed the GH-44464-arrow-result-status-on-rvalue-object branch from 2449343 to 5b5e898 Compare November 3, 2024 21:24
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Nov 3, 2024
@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Nov 4, 2024
@igor-anferov
Copy link
Author

@bkietz @pitrou @mapleFU, thank you very much for the review! Could you please let me know the next steps to get this merged? I looked for this information in the contributor’s guide but couldn’t find it.

@mapleFU
Copy link
Member

mapleFU commented Nov 5, 2024

Will merge this if no negative comment tonight

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants