Commit dcde6d37 authored by Marshall Cline's avatar Marshall Cline Committed by Facebook Github Bot

rvalueification of Future::within(...): 1/n

Summary:
This is part of "the great r-valuification of folly::Future":
* This is something we should do for safety in general.
* Several of folly::Future's methods are lvalue-qualified even though they act as though they are rvalue-qualified, that is, they provide a postcondition that says, in effect, callers should act as though the method invalidated its `this` object (regardless of whether that invalidation was actual or logical).
* This violates the C++ principle to "Express ideas directly in code" (see Core Guidelines), and generally makes it more confusing for callers as well as hiding the actual semantics from tools (linters, compilers, etc.).
* This dichotomy and confusion has manifested itself by some failures around D7840699 since lvalue-qualification hides that operation's move-out semantics - leads to some use of future operations that are really not correct, but are not obviously incorrect.
* The goal of rvalueification is to make sure methods that are logically rvalue-qualified are actually rvalue-qualified, which forces callsites to acknowledge that rvalueification, e.g., `std::move(f).within(...)` instead of `f.within(...)`. This syntactic change in the callsites forces callers to acknowledge the method's rvalue semantics.

Reviewed By: LeeHowes

Differential Revision: D9442173

fbshipit-source-id: 910914fd9da80627b335419542b5bb358b21f680
parent 9f748fb5
...@@ -428,7 +428,7 @@ FutureBase<T>::thenImplementation( ...@@ -428,7 +428,7 @@ FutureBase<T>::thenImplementation(
template <class T> template <class T>
template <typename E> template <typename E>
SemiFuture<T> SemiFuture<T>
FutureBase<T>::withinImplementation(Duration dur, E e, Timekeeper* tk) { FutureBase<T>::withinImplementation(Duration dur, E e, Timekeeper* tk) && {
struct Context { struct Context {
explicit Context(E ex) : exception(std::move(ex)) {} explicit Context(E ex) : exception(std::move(ex)) {}
E exception; E exception;
...@@ -1221,7 +1221,7 @@ Future<T> Future<T>::ensure(F&& func) && { ...@@ -1221,7 +1221,7 @@ Future<T> Future<T>::ensure(F&& func) && {
template <class T> template <class T>
template <class F> template <class F>
Future<T> Future<T>::onTimeout(Duration dur, F&& func, Timekeeper* tk) { Future<T> Future<T>::onTimeout(Duration dur, F&& func, Timekeeper* tk) {
return within(dur, tk).template thenError<FutureTimeout>( return std::move(*this).within(dur, tk).template thenError<FutureTimeout>(
[funcw = std::forward<F>(func)](auto const&) mutable { [funcw = std::forward<F>(func)](auto const&) mutable {
return std::forward<F>(funcw)(); return std::forward<F>(funcw)();
}); });
...@@ -1936,19 +1936,20 @@ Future<T> unorderedReduce(It first, It last, T initial, F func) { ...@@ -1936,19 +1936,20 @@ Future<T> unorderedReduce(It first, It last, T initial, F func) {
// within // within
template <class T> template <class T>
Future<T> Future<T>::within(Duration dur, Timekeeper* tk) { Future<T> Future<T>::within(Duration dur, Timekeeper* tk) && {
return within(dur, FutureTimeout(), tk); return std::move(*this).within(dur, FutureTimeout(), tk);
} }
template <class T> template <class T>
template <class E> template <class E>
Future<T> Future<T>::within(Duration dur, E e, Timekeeper* tk) { Future<T> Future<T>::within(Duration dur, E e, Timekeeper* tk) && {
if (this->isReady()) { if (this->isReady()) {
return std::move(*this); return std::move(*this);
} }
auto* exe = this->getExecutor(); auto* exe = this->getExecutor();
return this->withinImplementation(dur, e, tk) return std::move(*this)
.withinImplementation(dur, e, tk)
.via(exe ? exe : &InlineExecutor::instance()); .via(exe ? exe : &InlineExecutor::instance());
} }
......
...@@ -436,7 +436,7 @@ class FutureBase { ...@@ -436,7 +436,7 @@ class FutureBase {
thenImplementation(F&& func, futures::detail::argResult<isTry, F, Args...>); thenImplementation(F&& func, futures::detail::argResult<isTry, F, Args...>);
template <typename E> template <typename E>
SemiFuture<T> withinImplementation(Duration dur, E e, Timekeeper* tk); SemiFuture<T> withinImplementation(Duration dur, E e, Timekeeper* tk) &&;
}; };
template <class T> template <class T>
void convertFuture(SemiFuture<T>&& sf, Future<T>& f); void convertFuture(SemiFuture<T>&& sf, Future<T>& f);
...@@ -806,7 +806,7 @@ class SemiFuture : private futures::detail::FutureBase<T> { ...@@ -806,7 +806,7 @@ class SemiFuture : private futures::detail::FutureBase<T> {
template <class E> template <class E>
SemiFuture<T> within(Duration dur, E e, Timekeeper* tk = nullptr) && { SemiFuture<T> within(Duration dur, E e, Timekeeper* tk = nullptr) && {
return this->isReady() ? std::move(*this) return this->isReady() ? std::move(*this)
: this->withinImplementation(dur, e, tk); : std::move(*this).withinImplementation(dur, e, tk);
} }
/// Delay the completion of this SemiFuture for at least this duration from /// Delay the completion of this SemiFuture for at least this duration from
...@@ -1536,7 +1536,11 @@ class Future : private futures::detail::FutureBase<T> { ...@@ -1536,7 +1536,11 @@ class Future : private futures::detail::FutureBase<T> {
/// - Calling code should act as if `valid() == false`, /// - Calling code should act as if `valid() == false`,
/// i.e., as if `*this` was moved into RESULT. /// i.e., as if `*this` was moved into RESULT.
/// - `RESULT.valid() == true` /// - `RESULT.valid() == true`
Future<T> within(Duration, Timekeeper* = nullptr); Future<T> within(Duration, Timekeeper* = nullptr) &&;
auto within(Duration dur, Timekeeper* tk = nullptr) & {
return std::move(*this).within(dur, tk);
}
/// Throw the given exception if this Future does not complete within the /// Throw the given exception if this Future does not complete within the
/// given duration from now. The optional Timekeeper is as with /// given duration from now. The optional Timekeeper is as with
...@@ -1552,7 +1556,12 @@ class Future : private futures::detail::FutureBase<T> { ...@@ -1552,7 +1556,12 @@ class Future : private futures::detail::FutureBase<T> {
/// i.e., as if `*this` was moved into RESULT. /// i.e., as if `*this` was moved into RESULT.
/// - `RESULT.valid() == true` /// - `RESULT.valid() == true`
template <class E> template <class E>
Future<T> within(Duration, E exception, Timekeeper* = nullptr); Future<T> within(Duration, E exception, Timekeeper* = nullptr) &&;
template <class E>
auto within(Duration dur, E exception, Timekeeper* tk = nullptr) & {
return std::move(*this).within(dur, exception, tk);
}
/// Delay the completion of this Future for at least this duration from /// Delay the completion of this Future for at least this duration from
/// now. The optional Timekeeper is as with futures::sleep(). /// now. The optional Timekeeper is as with futures::sleep().
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment