Use perfect forward

[cyyever]

Description

Change some templates to use perfect forward. The forwarded items mostly argument annotations and other pybind11 objects so there should be benefits of moving them.

[wjakob]

It's a good default policy to use perfect forwarding. But in this case, it's not clear to me what the actual benefit is.

[rwgk]

In the meantime it crossed my mind: This PR doesn't add tests or modify any unit tests. That's not ideal.

My general rule of thumb: If there is no test addition or change, it's probably not worth spending time on it (whatever "it" may be).

To turn this around: Best practice is to write or (rarely) change a test first, then fix.

[cyyever]

@rwgk Grepping the source files to find out parameter packs. With respect to tests, there is already test coverage for cpp_function and other changes. However, before the PR the arguments were passed as const references, now that they are passed differently.

[rwgk]

@rwgk Grepping the source files to find out parameter packs. With respect to tests, there is already test coverage for cpp_function and other changes. However, before the PR the arguments were passed as const references, now that they are passed differently.

Of course.

What's missing in this PR: a test that only works with your changes.

Now, this PR clearly is a good change.

But it's also entirely inconsequential, and there is nothing that prevents the (theoretical) improvement from accidentally getting lost again in some refactoring work.

So this PR creates a disturbance (and a distraction for maintainers) without any tangible benefit.

General guideline:

If you believe something is important:

Always write a test FIRST that fails if the production code changes are reverted.

If you come to the conclusion it's not worth the time writing the tests: Drop the idea entirely.

There are exceptions to this, like fixing a bug for a situation that is extremely difficult to capture in a unit test. But this PR isn't in that category.

[dalboris]

Just randomly came across this, so I might as well provide my opinion.

Switching a function argument from a const ref (const T&) to a universal ref (templated T&&) makes two API changes:

1. The function now also accepts non-const references (e.g., int&)
2. The function now also accepts rvalue references (e.g., int&&)

The first point may or may not be desirable. Could it hide bugs further down the call chain, where now the function calls an overload that mutates the passed reference? Example:

void foo(const int& t) {  }
void foo(int& t) {  t = 42; }

template<typename T>
void bar(T&& t) {
    foo(std::forward<T>(t));
}

template<typename T>
void baz(const T& t) {
    foo(t);
}

int x = 1;
bar(x); // mutates x
baz(x); // does not mutate x

The second point is only useful for performance, but only if the argument is actually moved. Does the function creates resources from the arguments that outlive the function call, that can take advantage of stealing the resources given as arguments? If yes, then there can be a performance gain. Otherwise, if the function only uses its arguments, but never needs to copy it, then there is no performance gain to be had. I do not feel like knowing the internal of pybind11 enough to know in which case we are, but since Extra is so generic, I guess there might be cases where there are indeed copies that are done, that may benefit from moving instead.

[cyyever]

@dalboris I understand your concerns. However, given the examples, usage of universal ref actually reveals the underlying inconsistency in API design, in that two versions of the foo function try to do semantically different things with the argument. This is not the fault of universal ref, function overloading should be blamed instead. Nevertheless, in my opinion, universal ref was introduced in C++11 solely for the possible performance gains, with the extra complexity of reference resolution (from the perspective of the compiler and the developers). It's reasonable to add news tests according to the suggestions of @rwgk , which I will do ASAP. Then it's up to the community to decide whether to use universal ref.

[dalboris]

Nevertheless, in my opinion, universal ref was introduced in C++11 solely for the possible performance gains, with the extra complexity of reference resolution (from the perspective of the compiler and the developers).

No, rvalue references were introduced for performance gains. The different concept of universal references was introduced to write generic code that accepts either const ref, mutable ref, or rvalue ref, without the boilerplate of having to define three overloads for this.

Your commit has the goal to increase performance by allowing the functions to accept rvalue references. You do this by using universal references arguments. But this has the side effect that now, the functions also accept mutable references, which may or may not be desirable. I'm not saying whether or not it is desirable here. I'm just saying that it's one important thing to consider in the discussion.

[Skylion007]

@rwgk If we want unit tests, we just need to update clang-tidy and the static analysis in the latest versions will flag it. I think that's how @cyyever found these issues.

[rwgk]

@rwgk If we want unit tests, we just need to update clang-tidy and the static analysis in the latest versions will flag it.

That would make it a lot more interesting. @cyyever, did you use a newer clang-tidy? (Previously you just mentioned grepping.)

Jumping ahead, under the assumption that the latest clang-tidy flags the code changed under this PR:

There would be two parts that come to mind:

• Resolving the clang-tidy errors.
• Running global testing. (I'd need to ask someone at Google.)

The latter comes to mind because @dalboris wrote:

The first point may or may not be desirable. Could it hide bugs further down the call chain, where now the function calls an overload that mutates the passed reference?

Guessing is hard, global testing usually very conclusive.