bkietz commented on a change in pull request #9547:
URL: https://github.com/apache/arrow/pull/9547#discussion_r580503186
##########
File path: cpp/src/arrow/util/future.h
##########
@@ -493,6 +493,22 @@ class ARROW_MUST_USE_TYPE Future {
});
}
+ /// \brief Implicit constructor to create a future from a Result, enabling
use
+ /// of macros like ARROW_ASSIGN_OR_RAISE.
+ Future(Result<ValueType> res) : Future() { // NOLINT(runtime/explicit)
+ if (ARROW_PREDICT_TRUE(res.ok())) {
+ impl_ = FutureImpl::MakeFinished(FutureState::SUCCESS);
+ } else {
+ impl_ = FutureImpl::MakeFinished(FutureState::FAILURE);
+ }
+ SetResult(std::move(res));
+ }
+
+ /// \brief Implicit constructor to create a future from a Status, enabling
use
+ /// of macros like ARROW_RETURN_NOT_OK.
+ Future(Status s) // NOLINT(runtime/explicit)
Review comment:
```suggestion
Future(Status s) // NOLINT runtime/explicit
```
##########
File path: cpp/src/arrow/util/future.h
##########
@@ -493,6 +493,22 @@ class ARROW_MUST_USE_TYPE Future {
});
}
+ /// \brief Implicit constructor to create a future from a Result, enabling
use
+ /// of macros like ARROW_ASSIGN_OR_RAISE.
+ Future(Result<ValueType> res) : Future() { // NOLINT(runtime/explicit)
Review comment:
Some tools don't recognize the parenthesized form
```suggestion
Future(Result<ValueType> res) : Future() { // NOLINT runtime/explicit
```
##########
File path: cpp/src/arrow/util/future_test.cc
##########
@@ -392,6 +392,30 @@ TEST(FutureSyncTest, GetStatusFuture) {
}
}
+// Ensure the implicit convenience constructors behave as desired.
+TEST(FutureSyncTest, ImplicitConstructors) {
+ {
+ auto fut = ([]() -> Future<MoveOnlyDataType> {
+ return arrow::Status::Invalid("Invalid");
+ })();
+ AssertFailed(fut);
+ ASSERT_RAISES(Invalid, fut.result());
+ }
+ {
+ auto fut = ([]() -> Future<MoveOnlyDataType> {
+ return
arrow::Result<MoveOnlyDataType>(arrow::Status::Invalid("Invalid"));
+ })();
+ AssertFailed(fut);
+ ASSERT_RAISES(Invalid, fut.result());
+ }
Review comment:
Test case for the previous:
```suggestion
}
{
auto fut = ([]() -> Future<MoveOnlyDataType> { return
MoveOnlyDataType(42); })();
AssertSuccessful(fut);
}
```
##########
File path: cpp/src/arrow/util/future.h
##########
@@ -493,6 +493,22 @@ class ARROW_MUST_USE_TYPE Future {
});
}
Review comment:
For completeness, we could also support construction from ValueType:
```suggestion
}
/// \brief Implicit constructor to create a finished future from a value
Future(ValueType val) : Future() { // NOLINT runtime/explicit
impl_ = FutureImpl::MakeFinished(FutureState::SUCCESS);
SetResult(std::move(val));
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
