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

use rvalue-qual Future::get(): pass 5

Summary:
This is part of "the great r-valuification of folly::Future":

* This is something we should do for safety in general.
* Context: `Future::get(...)` means both `Future::get()` and `Future::get(Duration)`
* Using lvalue-qualified `Future::get(...)` has caused 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.
* Problems with `Future::get(...) &`: it moves-out the result but doesn't invalidate the Future - the Future remains (technically) valid even though it actually is partially moved-out. Callers can subsequently access that moved-out result via things like `future.get(...)`, `future.result()`, `future.value()`, etc. - these access an already-moved-out result which is/can be surprising.
* Reasons `Future::get(...) &&` is better: its semantics are more obvious and user-testable. It moves-out the Future, leaving it with `future.valid() == false`.

Reviewed By: djwatson

Differential Revision: D8711357

fbshipit-source-id: 7b6a4119f4427fbc779b1f9e1c0dd44762021894
parent 01920134
......@@ -36,18 +36,18 @@ TEST(AsyncFunc, manual_executor) {
TEST(AsyncFunc, value_lambda) {
auto lambda = [] { return 42; };
auto future = async(lambda);
EXPECT_EQ(42, future.get());
EXPECT_EQ(42, std::move(future).get());
}
TEST(AsyncFunc, void_lambda) {
auto lambda = [] { /*do something*/ return; };
auto future = async(lambda);
// Futures with a void returning function, return Unit type
EXPECT_EQ(typeid(Unit), typeid(future.get()));
EXPECT_EQ(typeid(Unit), typeid(std::move(future).get()));
}
TEST(AsyncFunc, moveonly_lambda) {
auto lambda = [] { return std::make_unique<int>(42); };
auto future = async(lambda);
EXPECT_EQ(42, *future.get());
EXPECT_EQ(42, *std::move(future).get());
}
......@@ -614,7 +614,7 @@ static void removeThreadTest() {
fe.setNumThreads(1);
// future::then should be fulfilled because there is other thread available
EXPECT_EQ(77, f->get());
EXPECT_EQ(77, std::move(*f).get());
// two thread should be different because then part should be rescheduled to
// the other thread
EXPECT_NE(id1, id2);
......@@ -786,7 +786,7 @@ static void WeakRefTest() {
.then([]() { burnMs(100)(); })
.then([&] { ++counter; });
}
EXPECT_THROW(f->get(), folly::BrokenPromise);
EXPECT_THROW(std::move(*f).get(), folly::BrokenPromise);
EXPECT_EQ(1, counter);
}
......
......@@ -119,7 +119,7 @@ TEST(Coro, FutureThrow) {
executor.drive();
EXPECT_TRUE(future.isReady());
EXPECT_THROW(future.get(), std::runtime_error);
EXPECT_THROW(std::move(future).get(), std::runtime_error);
}
coro::Task<int> taskRecursion(int depth) {
......
......@@ -199,5 +199,5 @@ TEST(Reduce, unorderedReduceFuture) {
ps[1].setValue(2.0);
ps[2].setValue(3.0);
EXPECT_EQ(1.0, f.get());
EXPECT_EQ(1.0, std::move(f).get());
}
......@@ -222,7 +222,7 @@ TEST(Timekeeper, semiFutureWithinAlreadyComplete) {
auto f = makeSemiFuture(42).within(one_ms).toUnsafeFuture().onError(
[&](FutureTimeout&) { return -1; });
EXPECT_EQ(42, f.get());
EXPECT_EQ(42, std::move(f).get());
}
TEST(Timekeeper, futureWithinFinishesInTime) {
......@@ -243,7 +243,7 @@ TEST(Timekeeper, semiFutureWithinFinishesInTime) {
.onError([&](FutureTimeout&) { return -1; });
p.setValue(42);
EXPECT_EQ(42, f.get());
EXPECT_EQ(42, std::move(f).get());
}
TEST(Timekeeper, futureWithinVoidSpecialization) {
......
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