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

[igor-anferov]

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: [C++] arrow::Result::status() should have rvalue-reference-qualified overload returning value instead of reference #44464

[github-actions]

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

[mapleFU]

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

[igor-anferov]

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

Will merge this if no negative comment tonight

[mapleFU]

Thanks all, merged

[conbench-apache-arrow]

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

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.