[PATCH] D41372: [libcxx] Fix transform_reduce mishandling move-only types, and nonstandard macro use in tests.

[Billy Robert O'Neal III via Phabricator via cfe-commits]

BillyONeal added a comment.

Do you folks want me to port this test case for move-only T into libcxx? (I added this to our test harness) Note that discussion on validity is ongoing on lists.

  #define _SILENCE_PARALLEL_ALGORITHMS_EXPERIMENTAL_WARNING
  #include <stdlib.h>
  #include <algorithm>
  #include <execution>
  #include <memory>
  #include <iterator>
  #include <numeric>
  #include <utility>
  #include <vector>
  #include <parallel_algorithms_utilities.hpp>
  #include <pmretvals.h>
  
  using namespace std;
  using namespace std::execution;
  
  void verify(const bool b) {
      if (!b) {
          exit(PM_TEST_FAIL);
      }
  }
  
  vector<unique_ptr<vector<unsigned int>>> get_move_only_test_data(const size_t testSize) {
      vector<unique_ptr<vector<unsigned int>>> testData;
      testData.reserve(testSize);
      for (size_t idx = 0; idx < testSize; ++idx) {
          testData.emplace_back(make_unique<vector<unsigned int>>(1, static_cast<unsigned int>(idx)));
      }
  
      return testData;
  }
  
  template<class ExPo>
  void test_case_move_only(const size_t testSize, ExPo&& exec) {
      auto testData = get_move_only_test_data(testSize);
      unique_ptr<vector<unsigned int>> result = transform_reduce(std::forward<ExPo>(exec),
          make_move_iterator(testData.begin()), make_move_iterator(testData.end()),
          make_unique<vector<unsigned int>>(),
          [](unique_ptr<vector<unsigned int>> lhs, unique_ptr<vector<unsigned int>> rhs) {
              lhs->insert(lhs->end(), rhs->begin(), rhs->end());
              return lhs;
          },
          [](unique_ptr<vector<unsigned int>> target) {
              verify(target->size() == 1); // should only be called directly on the input sequence
              target->back() *= 10;
              return target;
          });
  
      sort(result->begin(), result->end());
      for (size_t idx = 0; idx < testSize; ++idx) {
          verify((*result)[idx] == idx * 10);
      }
  }
  
  int main() {
      parallel_test_case(test_case_move_only<const sequenced_policy&>, seq);
      parallel_test_case(test_case_move_only<const parallel_policy&>, par);
      return PM_TEST_PASS;
  }



================
Comment at: include/numeric:245
     for (; __first1 != __last1; ++__first1, (void) ++__first2)
         __init = __b1(__init, __b2(*__first1, *__first2));
     return __init;
----------------
BillyONeal wrote:
> It's arguable that this should be _VSTD::move(__init); I asked SG1 / LEWG about that. (After all, if they're going to make inner_product do that this should too)
Any comment on whether these should have extra moves added @mclow.lists  ?


================
Comment at: test/std/numerics/numeric.ops/transform.reduce/transform_reduce_iter_iter_iter_init.pass.cpp:61
 
+inline MoveOnly operator+(const MoveOnly& lhs, const MoveOnly& rhs)
+{
----------------
mclow.lists wrote:
> BillyONeal wrote:
> > Should these go in MoveOnly.h ?
> I think so. While you're at it, you can remove the `friend` declaration in `MoveOnly` - that's a remnant.
Should I also add op== and op< while I'm in there for testing things like sort?


Repository:
  rL LLVM

https://reviews.llvm.org/D41372