accumulate compile error using a lambda with ref parameter

[cordarei]

Hi, I'm using this library in a project, and I encountered this problem. The following code doesn't compile (either on Clang 3.5.0 or gcc 4.9.1; my OS is Arch Linux):

#include <range/v3/numeric/accumulate.hpp>

void foo() {
  auto v = std::vector<std::string>{"foo", "bar", "baz"};
  auto s = ranges::accumulate(v, std::string{}, [](auto &a, auto const &b) { return a += b; })
}

It compiles if both parameters are by-value or const&, but it doesn't work if one or both is a reference. I can't figure out why it isn't working (maybe something to do with the Accumulateable concept?), but it seems like something that is reasonable to do.

Thanks in advance, and thanks for the library, it's very nice.

[meox]

hi,

have tou try declaring your lambda mutable?

[](auto &a, auto const &b) mutable { ... }

On 1 November 2014 15:23, Joseph Irwin notifications@github.com wrote:

Hi, I'm using this library in a project, and I encountered this problem.
The following code doesn't compile (either on Clang 3.5.0 or gcc 4.9.1; my
OS is Arch Linux):

#include <range/v3/numeric/accumulate.hpp>

void foo() {
auto v = std::vectorstd::string{"foo", "bar", "baz"};
auto s = ranges::accumulate(v, std::string{}, [](auto &a, auto const &b) { return a += b; })
}

It compiles if both parameters are by-value or const&, but it doesn't work
if one or both is a reference. I can't figure out why it isn't working
(maybe something to do with the Accumulateable concept?), but it seems like
something that is reasonable to do.

Thanks in advance, and thanks for the library, it's very nice.

—
Reply to this email directly or view it on GitHub
#63.

GL
http://www.meocci.it

[asutton]

Note that with std::accumulate, the binary operator is not supposed to
have side effects. So, if you tried that with the current standard library,
it would not be considered reasonable, but undefined behavior.

That's statement is a bit too strong. Side effects like allocating memory
or writing to a file are just fine. Otherwise, this is is exactly the right.

Andrew

[ericniebler]

Agree; the accumulate function should not be mutating the underlying sequence. But this highlights a larger issue with the library's concept checking, and I hope that you can shed some light on this, @asutton. It seems OK to want to mutate the input sequence via std::for_each and a function that takes by non-const ref, but such a function doesn't get past the concept checks. That's because in N3351, for_each is specified as:

template<InputIterator I, Semiregular F>
    requires Function<F, ValueType<I>>
F for_each(I first, I last, F f)

I interpret this as: F must be able to take an rvalue of type ValueType<I>. That doesn't work if F is expecting a non-const ref. Either I am interpreting it wrong, or else a lot of people are going to be unhappy.

[asutton]

template<InputIterator I, Semiregular F>
requires Function<F, ValueType>
F for_each(I first, I last, F f)

I interpret this as: F must be able to take an rvalue of type
ValueType. That doesn't work if F is expecting a non-const ref. Either
I am interpreting it wrong, or else a lot of people are going to be unhappy.

You have to look at the arguments used to write the requirement. For a
single argument case it would look like this:

template
concept bool Unary = requires(F f, Arg arg) { f(arg); }

So f would accept an lvalue ref, const lvalue ref, or rvalue.

You could try writing "Function<F, const ValueType>", but that can be a
bit weird. If I is not a const iterator, I'm not sure the constraint would
be satisfied.

Andrew

[ericniebler]

Ooooh. I see. Your code is doing:

template<typename F, typename Arg>
concept bool Unary = requires(F f, Arg arg) { f(arg); }

And my concept checking is doing the moral equivalent of:

template<typename F, typename Arg>
concept bool Unary = requires { declval<F>()(declval<Arg>()); }

That's very different. I need to change it to:

template<typename F, typename Arg>
concept bool Unary = requires { declval<F &>()(declval<Arg &>()); }

Wow, that's going to break a ton of stuff. :-P

[asutton]

Ooooh. I see. Your code is doing:

template<typename F, typename Arg>
concept bool Unary = requires(F f, Arg arg) { f(arg); }

And my concept checking is doing the moral equivalent of:

template<typename F, typename Arg>
concept bool Unary = requires { declval()(declval()); }

Yeah... it's subtle difference, but a big difference :) I think, whenever I
wrote constraints in Origin, I always wrote the checks against a function
like this:

auto check(F f, Arg arg) -> decltype(f(arg));

And that mapped nicely into the way that Alex and Bjarne wanted to write
constraints in n3351.

I think our basic operating system is that when you write a constraint use
the name of a type (T or const T), then you allow the greatest amount of
flexibility. When you want to be specific, you use T&, const T&, or T&&.
Those show up in the assignment requirements, I think.

Andrew

[ericniebler]

I always wrote the checks against a function like this:

auto check(F f, Arg arg) -> decltype(f(arg));

Where F and Arg are not deduced, I take it.

When you want to be specific, you use T&, const T&, or T&&.

Your simple model works for all but the last one. That one needs special handling because any named thingy is an lvalue. My earlier suggestion of just using declval<T &>() is wrong for T&& also. It'll take some thought.

Anyway, thanks. That clears up a lot for me.

[cordarei]

I think I mostly understand; the lambda doesn't meet the concept requirements as they are written because Invokable (or Function) expects its arguments to be able to bind to rvalues because declval() returns an rvalue. And while I still think that taking the 'init' parameter as T& satisfies the requirement of not modifying the range, looking at cppreference.com it seems the standard library expects 'op' to behave like 'R op(T const&, U const&)' and I understand that it may not be worth changing that.

I certainly didn't expect to provoke such an involved discussion; hope I didn't let any genies out of their bottles.

[ericniebler]

After [e1db4ec], your code will compile as-is. I still think it's a bad idea. :-)