[PATCH] D33997: Implement the non-execution policy versions of `reduce` and `transform_reduce` for C++17

[Bryce Adelstein Lelbach via Phabricator via cfe-commits]

wash added a comment.

Suppose you have:

  struct A {};
  struct B {};
  
  A operator+(A, B);
  
  std::vector<B> v;
  std::reduce(v.begin(), v.end(), A{});

The implementation in this patch works fine for the above case - as would `accumulate`, which it is equivalent to. However, `reduce` requires that "All of `binary_op(init, *first)`, `binary_op(*first, init)`, `binary_op(init, init)`, and `binary_op(*first, *first)` shall be convertible to T.", as all those operations may be needed for the parallel execution-policy overload of `reduce` (the requirement applies to the non-execution-policy overload as well).

E.g. in the above example, these operations are also required by `reduce`, even though the non-parallel implementation does not need them:

  A operator+(B, A);
  A operator+(A, A);
  A operator+(B, B);

Should the non-parallel implementation of `reduce` static_assert or SFINAE away when these requirements are not met? I think it might be desirable to ensure that if this won't compile:

  std::reduce(std::par, v.begin(), v.end(), A{});

Then this will also not compile:

  std::reduce(v.begin(), v.end(), A{});

(And the spec seems to suggest this too).



================
Comment at: include/numeric:154
+    return reduce(__first, __last, 
+       typename iterator_traits<_InputIterator>::value_type{}, _VSTD::plus<>());
+}
----------------
In the spec, this overload of `reduce` is described as equivalent to `return reduce(std::forward<ExecutionPolicy>(exec), first, last, typename iterator_traits<ForwardIterator>::value_type{});`.  The overload that it calls (the three argument version that omits a binary operation) just forwards to the four-argument reduce, adding the `plus<>()` argument.

Is there a reason you wanted to avoid the extra layer of function call indirection (it should be inlined and optimized away, right)? What you have seems perfectly fine, I'm just curious though.


================
Comment at: test/std/numerics/numeric.ops/reduce/reduce_iter_iter.pass.cpp:37
+    unsigned sa = sizeof(ia) / sizeof(ia[0]);
+    test(Iter(ia), Iter(ia), 0);
+    test(Iter(ia), Iter(ia+1), 1);
----------------
 Just to confirm, this should be 0 because the "default" init value is `iterator_traits<_InputIterator>::value_type{}`, and `int{}` gives you a determinate result (as opposed to `int()` which would not), correct?


https://reviews.llvm.org/D33997