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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked an inline comment as done.
zoecarver added a comment.

> It's expecting users to provide operator+(T&&, T const&) and operator-(T const&, T&&) that move from the first and second arguments respectively.

It's not expecting users to do this, its //allowing// them to. Rvalue references can turn into lvalue references implicitly but not the other way around. My tests error when an rvalue is passed to an operator expecting an lvalue, but if you tried to use the same method to tests the opposite thing it would not work. In other words, if I used this in my tests, the program would still "work":

  force_move operator+(force_move const&, force_move const&)

This paper allows users to have huge speed improvements while not breaking code for most people.



================
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());
+}
----------------
EricWF wrote:
> zoecarver wrote:
> > mclow.lists wrote:
> > > I don't see what this code is testing.  You set no values in the array; nor do you test the results.
> > This ensures that the left-hand side of the addition operator is an rvalue. 
> A better name would be `ensure_move`, or `rvalue_addable`.
> 
> Also, why is the rvalue `const`?
> 
> 
I talked with @mclow.lists about this a bit; I am going to re-work these tests to use `TrackedValue` (or something similar) and add some tests for `std::string` (because that was one of the paper's main focuses).

> Also, why is the rvalue const?
Because it's never modified. It would normally be useless because it cannot be moved from, but here it's purpose is only in the argument type, so I think it's okay to copy it. I can change it if you want.


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

https://reviews.llvm.org/D61170