Add bugprone-move-forwarding-reference check to clang-tidy

[phate]

No description provided.

[haved]

@phate the compiler really didn't like the use of std::forward, but I'm not immediately sure exactly why, from looking at the errors

[haved]

Is there a reason why commutative_pairwise_reduce takes a Container && and not just a Container or const Container &? Changing to one of them (depending on if modifications are needed) would likely stop clang-tidy from suggesting forward

[phate]

@haved Fixed it now. I forgot to compile the first time.

[haved]

@phate I read up on the rules surrounding

template<typename T>
void x(T && t)

and I do not think it makes sense to use &&. If we just use Container args and return args, everything should optimize nicely

[phate]

@haved Could you elaborate or have a link for me?

@caleridas Do you have an opinion here? The code was originally written by you (long time ago).

[haved]

Reading the documentation of std::forward, from here.

When we use the pattern

template<typename T>
T func(T&& t) {
    t.push_back(t.size());
    return std::forward<T>(t);
}

t is what's called a "forwarding reference".

If an rvalue is passed into func, the type of T becomes the plain type, without any reference or const. This means the return value of func is a completely new instance of T.

When func is called with an lvalue, T becomes a reference type, which means the parameter becomes T& && which condenses down to T&. The return value is a reference to the argument.

If func is called with a const lvalue, T becomes a const reference type, and the argument becomes a const T& && == const T&, which will be problematic in the above example due to the push_back, and not compile.

It is honestly a bit fun to have functions like this, because it makes it possible to do operations in-place without any copying or moving, but also to take a temporary value and construct a new result type if needed.

The downside is that the function does not compile if it is given a const reference, which can be a bit surprising when the function seemingly is capable of returning a new container. Then again, a compile error is much more preferable than introducing subtle bugs.

The alternative I was suggesting is

template<typename Container, typename Reductor>
Container
commutative_pairwise_reduce(Container args, const Reductor & reductor) {
    ...
    return args; // No need for std::move() or std::forward(), let named return value optimization do its thing

This will always compile, and if you have a list of arguments you want to change "in place", you can call it like

list = commutative_pairwise_reduce(std::move(list), reductor);

which will perform one move-construction, but that's it.

For reference, every call to commutative_pairwise_reduce and pairwise_reduce currently uses std::move() for its parameter already.

I personally think this approach is a bit nicer, as it reduces the amount of C++ mental gymnastics one must go through. For the use cases we have, I think the total amount of move-construction should be the same, since we get named return value optimization.

What do you think @phate, @caleridas?

[phate]

@haved Thanks a lot for the write-up and condensing it down for me. I agree with you. @caleridas Unless there are any objections from your side, I will change it to @haved 's suggestion.

[phate]

@caleridas Opinion?

[caleridas]

@caleridas Opinion?

The explanation above by @haved s accurate, and I concur that && should not be used (unless you have a function that should be templated on "constness" of argument).

[caleridas]

Would suggest removing the && and just use plain value. It is up to caller to move values into it if it wants to pass ownership instead of copying.

[phate]

Fixed