[GitHub] [arrow] westonpace commented on a change in pull request #10205: ARROW-12004: [C++] Result is annoying

[GitBox <gi...@apache.org>]

westonpace commented on a change in pull request #10205:
URL: https://github.com/apache/arrow/pull/10205#discussion_r625263718



##########
File path: cpp/src/arrow/util/thread_pool_test.cc
##########
@@ -189,11 +189,7 @@ TEST_P(TestRunSynchronously, SpawnMoreNested) {
   auto top_level_task = [&](Executor* executor) -> Future<> {
     auto fut_a = DeferNotOk(executor->Submit([&] { nested_ran++; }));
     auto fut_b = DeferNotOk(executor->Submit([&] { nested_ran++; }));
-    return AllComplete({fut_a, fut_b})
-        .Then([&](const Result<arrow::detail::Empty>& result) {
-          nested_ran++;
-          return result;
-        });
+    return AllComplete({fut_a, fut_b}).Then([&]() { nested_ran++; });

Review comment:
       What a continuation returns is a separate decision from what it accepts.  However, returning a result here was essentially a no-op before and would still be one now.
   
   The first argument (or only argument in the 1-argument overload) to `Then` will only be called on success and it is given `result.ValueOrDie()` so accepting a `Result` is somewhat counter-productive.  This is different from `AddCallback` which is called on success or failure.
   
   It would be good to make it a compile error to accept a `Result<T>` in all cases of `Then` to avoid this confusion.  The current state of the PR will make a compile error for `Future<Empty>::Then` (which is why this was caught) so I'll update it to catch the `Future<T>::Then` case as well.




-- 
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:
users@infra.apache.org