Should you pass a callable object as a sink argument by std::function or by a universal reference?-CodePudding

Consider the following code:

#include <functional>
#include <iostream>

struct S1
{
    S1(const std::function<void()>& _func) : func{_func} {}

    std::function<void()> func;
};

struct S2
{
    template<typename F>
    S2(F&& _func) : func{std::forward<F>(_func)} {}

    std::function<void()> func;
};

int main()
{
    S1 s1{[](){ std::cout << "S1 function called\n"; }};
    S2 s2{[](){ std::cout << "S2 function called\n"; }};
    
    s1.func();
    s2.func();

    return 0;
}

I'm trying to understand what are the design benefits/drawbacks of each of the 2 constructor implementations. Also, are there any performance differences between the two.

My main question is whether one should accept a std::function object or a universal reference to a callable when one knows the signature of the callable and stores it as a std::function in the class?

From what I've heard universal reference is the preferred approach because it provides the best opportunity for the compiler to do inlining.

However, I can think of some drawbacks of using an universal reference:

it can conflict with other constructors taking the same number of arguments, so it should be guarded using SFINAE or C 20 concepts. (This is a library problem, so not a big deal)
it is less explicit about what kind of callables it expects (can also be constrained using concepts). (This could be considered a user problem)
as a result of the previous drawback, the point of error in case of unfitting callable is moved from the call site to the member initialization site, which is much harder to understand. (This is definitely a user problem)

I guess my real question is: Should you expose the std::function nature of the class in the constructor interface or this is an implementation detail which must remain hidden?

CodePudding user response：

The first example will always copy, even if that could be avoided so it is suboptimal.

The 2nd example has the drawbacks you mentioned.

The recommended way is to take by value and move:

struct S3
{
    S3(std::function<void()> _func) : func{std::move(_func)} {}

    std::function<void()> func;
};

Now this might seem counterintuitive because you take by value an object that is potentially expensive to copy. But that is not a problem. Let's analyse the possible scenarios:

the caller cannot give you movable object. In this case a copy is unavoidable. This results in a copy followed by a move:
```
auto l = []() {};
S3 s4{l};
l(); 
```
the caller gives you an xvalue. In this case there are 2 moves involved:
```
auto l = []() {};
S3 s4{std::move(l)};
// no more uses of l
```
the caller gives you a prvalue. This results in 1 or 2 moves. Since C 17 with the new temporary materialization rule you are guaranteed just one move.
```
S3 s4{[]() {}};
```

As you can see, even if you take by value you don't do any unnecessary copy operations and the premise is that the moves are cheap.

CodePudding user response：

TL;DR: you should use a universal reference constructor with concepts.

Moves are not always cheap. Since functors usually have a hefty amount of state and moves are equivalent to copies (without the PImpl idiom). One might argue that in modern c , functors are rarely used and when the PImpl idiom is used, moves and copies alike become cheap.

However, lambdas are functors behind the scenes and I don't think it is easy or trivial to implement the PImpl idiom with lambdas. This means that everything that is captured by the lambda is "moved", which for primitive types and anything that is efficient enough not to use dynamic allocation (such as std::string implementations that use the small string optimization) is not any cheaper than a copy.

What I am saying is that all moves and copies that it is possible to avoid should be avoided. Now, in this limited example, you don't take state. However, many functors/lambdas do, and your example would work with them.

While @bolov 's solution is easy and simple to make, perhaps it is better to use universal references and concepts. Consider the following:

template <class F, class R, class... Args >
concept invocable_r =
  requires(F&& f, Args&&... args) {
    {std::forward<F>(f)(std::forward<Args>(args)...)} -> std::same_as<R>;
  };
struct S2
{
    template<typename F> requires invocable_r<F, void>
    S2(F&& _func) : func{std::forward<F>(_func)} {}

    std::function<void()> func;
};
struct S1{
    S1(std::function<void()> f): func{f}{}
    std::function<void()> func;
};

Though S1 certainly doesn't take as many characters to type, it will almost always incur an additional move.

But what about S2? doesn't it have the problems you mentioned? Well, let's go over them:

it can conflict with other constructors taking the same number of arguments, so it should be guarded using SFINAE or C 20 concepts. (This is a library problem, so not a big deal)

I don't consider having to use concepts to be a problem, though I will talk about the problem with SFINAE.

it is less explicit about what kind of callables it expects (can also be constrained using concepts). (This could be considered a user problem)

Usually, the definition or declaration of something is separated from its usage. Also, a user will typically be looking through documentation of code, not the actual code itself.

as a result of the previous drawback, the point of error in case of unfitting callable is moved from the call site to the member initialization site, which is much harder to understand. (This is definitely a user problem).

Now without concepts, I would completely agree with you. Even with SFINAE, some error messages get really nasty.

However consider what error message the following line of code generates in OnlineGDB:

S1 s1{3};

main.cpp:34:12: error: no matching function for call to ‘S1::S1()’
   34 |     S1 s1{3};
      |            ^
main.cpp:29:5: note: candidate: ‘S1::S1(std::function)’
   29 |     S1(std::function<void()> f): func{f}{}
      |     ^~
main.cpp:29:30: note:   no known conversion for argument 1 from ‘int’ to ‘std::function’
   29 |     S1(std::function<void()> f): func{f}{}
      |        ~~~~~~~~~~~~~~~~~~~~~~^
main.cpp:28:8: note: candidate: ‘S1::S1(const S1&)’
   28 | struct S1{
      |        ^~
main.cpp:28:8: note:   no known conversion for argument 1 from ‘int’ to ‘const S1&’
main.cpp:28:8: note: candidate: ‘S1::S1(S1&&)’
main.cpp:28:8: note:   no known conversion for argument 1 from ‘int’ to ‘S1&&’

As compared with:

S2 s2{3};

main.cpp:35:12: error: no matching function for call to ‘S2::S2()’
   35 |     S2 s2{3};
      |            ^
main.cpp:24:5: note: candidate: ‘S2::S2(F&&) [with F = int]’
   24 |     S2(F&& _func) : func{std::forward<F>(_func)} {}
      |     ^~
main.cpp:24:5: note:   constraints not satisfied
main.cpp:16:9: note: within ‘template concept const bool invocable_r [with F = int; R = void; Args = {}]’
   16 | concept invocable_r =
      |         ^~~~~~~~~~~
main.cpp:16:9: note:     with ‘int&& f’
main.cpp:16:9: note: the required expression ‘same_as(f)((forward)(args)...)), R>’ would be ill-formed
main.cpp:21:8: note: candidate: ‘S2::S2(const S2&)’
   21 | struct S2
      |        ^~
main.cpp:21:8: note:   no known conversion for argument 1 from ‘int’ to ‘const S2&’
main.cpp:21:8: note: candidate: ‘S2::S2(S2&&)’
main.cpp:21:8: note:   no known conversion for argument 1 from ‘int’ to ‘S2&&’

While the second is slightly bigger, it could be argued that if one looked at the first part of the error message, it is a whole lot clearer:

main.cpp:35:12: error: no matching function for call to ‘S2::S2()’
   35 |     S2 s2{3};
      |            ^
main.cpp:24:5: note: candidate: ‘S2::S2(F&&) [with F = int]’
   24 |     S2(F&& _func) : func{std::forward<F>(_func)} {}
      |     ^~
main.cpp:24:5: note:   constraints not satisfied

"constraints not satisfied" is very clear indeed.