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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 3 inline comments as done.
zoecarver added inline comments.


================
Comment at: include/numeric:277
+            __t = _VSTD::move(__t) + *__first;
+            *__result = _VSTD::move(__t);
         }
----------------
mclow.lists wrote:
> 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`.
Yes, you're completely right. I will update this and the other instances of this issue below. 


================
Comment at: include/numeric:295
         {
-            __t = __binary_op(__t, *__first);
-            *__result = __t;
+            __t = __binary_op(_VSTD::move(__t), *__first);
+            *__result = _VSTD::move(__t);
----------------
mclow.lists wrote:
> Same comment as above. You're relying on the value of a moved-from object.
On the other two lines (292 and 296) not 295, right?


================
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);
----------------
zoecarver wrote:
> mclow.lists wrote:
> > And here.
> > For every iterator i in [first + 1, last) in order, creates an object val whose type is InputIterator’s value type, initializes it with *i, computes val - **std::move(acc)** or binary_op(val, **std::move(acc))**, assigns the result to *(result + (i - first)), and move assigns from val to acc.
> 
> Maybe I am misunderstanding what the paper is saying. While the above call to `std::move` is not specified in the paper, I think the one on line 425 is. 
This function might need some re-working to adhere to the standard.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D61170