-
Notifications
You must be signed in to change notification settings - Fork 3.8k
GH-46791: [C++] Add Status::OrElse
, IntoStatus<T>
and ToStatus
#46792
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
Conversation
|
@zanmato1984 @bkietz @paleolimbot What do you think? |
Ah, clang is being picky.
|
1c2266a
to
e2cd325
Compare
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.
These changes seems reasonable to me! The behaviour seems to line up exactly with Rust's .or_else()
and I agree the macro was difficult to understand. The general maintenance around status handling is also great!
You can guess it's intended :) |
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 looks nice! A few nits.
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.
LGTM, just some nits and an idea for an alternate approach
Status::OrElse
Status::OrElse
, IntoStatus<T>
and ToStatus
@github-actions crossbow submit -g cpp |
This comment was marked as outdated.
This comment was marked as outdated.
@github-actions crossbow submit -g cpp |
Revision: 65723c8 Submitted crossbow builds: ursacomputing/crossbow @ actions-dcfad22df7 |
@zanmato1984 @paleolimbot @bkietz I've tried to address all your comments and suggestions, can you take a look again? |
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 is a nice improvement on the ergonomics of working with the Status
! I took a look through for anything out of place although some of the C++ metaprogramming is over my head here 🙂
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.
LGTM
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 0140089. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
Rationale for this change
In #46711 (comment) it was mentioned that the macro
RETURN_NOT_OK_ELSE
is confusing and can easily be misunderstood. We would like a better way to conditionally chain error-handling code if a Status does not indicate success.What changes are included in this PR?
IntoStatus<T>
that can be implemented to provide conversions from other error-like typesToStatus
function that calls the aforementioned type traitStatus::OrElse
method that calls a functor on errorRETURN_NOT_OK_ELSE
macroAre these changes tested?
Yes.
Are there any user-facing changes?
No, the
RETURN_NOT_OK_ELSE
was not supposed to be called by third-party code as it's not prefixed withARROW_
.