arrow icon indicating copy to clipboard operation
arrow copied to clipboard

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

Open igor-anferov opened this issue 1 year ago • 1 comments

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.

  • GitHub Issue: #44464

igor-anferov avatar Oct 19 '24 20:10 igor-anferov

:warning: GitHub issue #44464 has been automatically assigned in GitHub to PR creator.

github-actions[bot] avatar Oct 19 '24 20:10 github-actions[bot]

@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.

igor-anferov avatar Nov 05 '24 00:11 igor-anferov

Will merge this if no negative comment tonight

mapleFU avatar Nov 05 '24 02:11 mapleFU

Thanks all, merged

mapleFU avatar Nov 06 '24 08:11 mapleFU

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 6decf1cca077488ec58aadd5a0a43408342ab612.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 29 possible false positives for unstable benchmarks that are known to sometimes produce them.