[libcxx-commits] [PATCH] D61170: Use std::move in numeric algorithms

[Marshall Clow via Phabricator via libcxx-commits]

mclow.lists requested changes to this revision.
mclow.lists added inline comments.
This revision now requires changes to proceed.


================
Comment at: include/numeric:277
+            __t = _VSTD::move(__t) + *__first;
+            *__result = _VSTD::move(__t);
         }
----------------
zoecarver wrote:
> Using `std::move` in places like this, while not specified in the standard (or paper), also improves performance a lot (about 7x in my tests). I think it would be good to try to find other places in numeric/algorithms where values could be moved instead of copied (because there is no downside and a lot of performance gains).
I don't think this is correct.
On line #273, you move from `__t`, but then on line #276, you use the value of `__t`.


================
Comment at: include/numeric:295
         {
-            __t = __binary_op(__t, *__first);
-            *__result = __t;
+            __t = __binary_op(_VSTD::move(__t), *__first);
+            *__result = _VSTD::move(__t);
----------------
Same comment as above. You're relying on the value of a moved-from object.


================
Comment at: include/numeric:425
             typename iterator_traits<_InputIterator>::value_type __t2(*__first);
-            *__result = __t2 - __t1;
+            *__result = _VSTD::move(__t2) - __t1;
             __t1 = _VSTD::move(__t2);
----------------
And here.


================
Comment at: include/numeric:445
             typename iterator_traits<_InputIterator>::value_type __t2(*__first);
-            *__result = __binary_op(__t2, __t1);
+            *__result = __binary_op(_VSTD::move(__t2), __t1);
             __t1 = _VSTD::move(__t2);
----------------
And here.


================
Comment at: test/std/numerics/numeric.ops/accumulate/accumulate_op.pass.cpp:40
+    std::accumulate(arr, arr + size, force_move());
+    std::accumulate(arr, arr + size, force_move(), force_move());
+}
----------------
I don't see what this code is testing.  You set no values in the array; nor do you test the results.


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61170/new/

https://reviews.llvm.org/D61170