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

[Zoe Carver via Phabricator via libcxx-commits]

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


================
Comment at: include/numeric:425
+            typename iterator_traits<_InputIterator>::value_type __val(*__first);
+            *__result = __val - _VSTD::move(__acc);
+            __acc = _VSTD::move(__val);
----------------
EricWF wrote:
> zoecarver wrote:
> > I can't find a good way to test that `__acc` is not read after being moved but before being re-assigned. If you have any ideas, let me know. Otherwise, I think it will be okay as-is. 
> You probably don't need a test for that exact bit of behavior.
> 
That's what I was thinking, glad you agree. 


================
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:
> > 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.
> If the purpose is the argument type, we want the parameter to match it.
> 
> Imagine if we wrote a bug where we passed `int&&` as `const int&&`. These tests wouldn't catch it.
So I should make two overloads? I am not really understanding what you are asking. A non-const argument can be passed as a const argument if it is not modified (which it isn't), so I don't think a bug would be created here because of the const-ness. Take this for example:
```
struct pred
{
	// operator which takes const parameters
	bool operator()(bool const& a, bool const& b) { return a == b; }
};

int main()
{
	auto p = pred { };
	const bool t = true; // const parameter
	bool f = false; // non-const parameter
	assert(p(true, true));
	assert(p(t, t));
	assert(p(f, f)); // both are fine because `f` is implicitly converted to a const type
}
```


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

https://reviews.llvm.org/D61170