westonpace commented on a change in pull request #10258:
URL: https://github.com/apache/arrow/pull/10258#discussion_r643634671
##########
File path: cpp/src/arrow/util/future.h
##########
@@ -453,30 +480,35 @@ class Future {
/// cyclic reference to itself through the callback.
template <typename OnComplete>
typename
std::enable_if<!detail::first_arg_is_status<OnComplete>::value>::type
- AddCallback(OnComplete on_complete) const {
+ AddCallback(OnComplete on_complete,
+ CallbackOptions opts = CallbackOptions::Defaults()) const {
// We know impl_ will not be dangling when invoking callbacks because at
least one
// thread will be waiting for MarkFinished to return. Thus it's safe to
keep a
// weak reference to impl_ here
struct Callback {
- void operator()() && { std::move(on_complete)(weak_self.get().result());
}
- WeakFuture<T> weak_self;
+ void operator()(const FutureImpl& impl) && {
+
std::move(on_complete)(*static_cast<Result<ValueType>*>(impl.result_.get()));
+ }
OnComplete on_complete;
};
- impl_->AddCallback(Callback{WeakFuture<T>(*this), std::move(on_complete)});
+ impl_->AddCallback(Callback{std::move(on_complete)}, opts);
}
/// Overload for callbacks accepting a Status
template <typename OnComplete>
typename std::enable_if<detail::first_arg_is_status<OnComplete>::value>::type
- AddCallback(OnComplete on_complete) const {
+ AddCallback(OnComplete on_complete,
+ CallbackOptions opts = CallbackOptions::Defaults()) const {
static_assert(std::is_same<internal::Empty, ValueType>::value,
"Callbacks for Future<> should accept Status and not
Result");
struct Callback {
- void operator()() && { std::move(on_complete)(weak_self.get().status());
}
- WeakFuture<T> weak_self;
+ void operator()(const FutureImpl& impl) && {
+ std::move(on_complete)(
+ static_cast<Result<ValueType>*>(impl.result_.get())->status());
Review comment:
It is a method on `Future` (named `GetResult`) but `FutureImpl` is
type-erased and so it has no reference to `ValueType`. If it were a `.cc` file
I could extract it into an anonymous function but no luck there because of
templates. Would it be acceptable to create a method `GetResultFromFutureImpl`
inside of `arrow::detail`? Or is there some other trick I can use?
--
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]
