westonpace commented on a change in pull request #10205: URL: https://github.com/apache/arrow/pull/10205#discussion_r631577351
########## File path: cpp/src/arrow/util/future.h ########## @@ -322,56 +341,98 @@ class ARROW_MUST_USE_TYPE Future { return impl_->Wait(seconds); } - // Producer API + protected: + void InitializeFromResult(Result<ValueType> res) { + if (ARROW_PREDICT_TRUE(res.ok())) { + impl_ = FutureImpl::MakeFinished(FutureState::SUCCESS); + } else { + impl_ = FutureImpl::MakeFinished(FutureState::FAILURE); + } + SetResult(std::move(res)); + } - /// \brief Producer API: mark Future finished - /// - /// The Future's result is set to `res`. - void MarkFinished(Result<ValueType> res) { DoMarkFinished(std::move(res)); } + void Initialize() { impl_ = FutureImpl::Make(); } - /// \brief Mark a Future<> completed with the provided Status. - template <typename E = ValueType, typename = typename std::enable_if< - std::is_same<E, detail::Empty>::value>::type> - void MarkFinished(Status s = Status::OK()) { - return DoMarkFinished(E::ToResult(std::move(s))); + Result<ValueType>* GetResult() const { + return static_cast<Result<ValueType>*>(impl_->result_.get()); } - /// \brief Producer API: instantiate a valid Future - /// - /// The Future's state is initialized with PENDING. If you are creating a future with - /// this method you must ensure that future is eventually completed (with success or - /// failure). Creating a future, returning it, and never completing the future can lead - /// to memory leaks (for example, see Loop). - static Future Make() { - Future fut; - fut.impl_ = FutureImpl::Make(); - return fut; + void SetResult(Result<ValueType> res) { + impl_->result_ = {new Result<ValueType>(std::move(res)), + [](void* p) { delete static_cast<Result<ValueType>*>(p); }}; } - /// \brief Producer API: instantiate a finished Future - static Future MakeFinished(Result<ValueType> res) { - Future fut; - if (ARROW_PREDICT_TRUE(res.ok())) { - fut.impl_ = FutureImpl::MakeFinished(FutureState::SUCCESS); + void DoMarkFinished(Result<ValueType> res) { + SetResult(std::move(res)); + + if (ARROW_PREDICT_TRUE(GetResult()->ok())) { + impl_->MarkFinished(); } else { - fut.impl_ = FutureImpl::MakeFinished(FutureState::FAILURE); + impl_->MarkFailed(); } - fut.SetResult(std::move(res)); - return fut; } - /// \brief Make a finished Future<> with the provided Status. - template <typename E = ValueType, typename = typename std::enable_if< - std::is_same<E, detail::Empty>::value>::type> - static Future<> MakeFinished(Status s = Status::OK()) { - return MakeFinished(E::ToResult(std::move(s))); + void CheckValid() const { +#ifndef NDEBUG + if (!is_valid()) { + Status::Invalid("Invalid Future (default-initialized?)").Abort(); + } +#endif + } + + explicit FutureBase(std::shared_ptr<FutureImpl> impl) : impl_(std::move(impl)) {} + + std::shared_ptr<FutureImpl> impl_; + + friend class FutureWaiter; + friend struct detail::ContinueFuture; +}; + +template <typename T> +class ARROW_MUST_USE_TYPE Future : public FutureBase<T> { + public: + using ValueType = T; + using SyncType = Result<T>; + static constexpr bool is_empty = false; + + Future() = default; + + /// \brief Returns an rvalue to the result. This method is potentially unsafe + /// + /// The future is not the unique owner of the result, copies of a future will + /// also point to the same result. You must make sure that no other copies + /// of the future exist. Attempts to add callbacks after you move the result + /// will result in undefined behavior. + Result<ValueType>&& MoveResult() { + FutureBase<T>::Wait(); + return std::move(*FutureBase<T>::GetResult()); + } + + /// \brief Wait for the Future to complete and return its Result (or Status for an empty + /// future) + /// + /// This method is useful for general purpose code converting from async to sync where T + /// is a template parameter and may be empty. + const SyncType& to_sync() const { return FutureBase<T>::result(); } Review comment: So based on the other comment this will return either `result()` or `status()` depending on `T`. It's the opposite of the "result-to-future" forwarding primitive. Note: after switching to one class this became a standalone static method `FutureToSync`. -- 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: us...@infra.apache.org