Perfect forwarding results in an error I don't understand

Question

This is a follow-up to this question where I was encouraged to use perfect forwarding in a variadic template operator()(...) implementation. Here's my observer pattern which I want to use to call free functions and member functions with variadic arguments:

#ifndef _SIGNALS_H_
#define _SIGNALS_H_

#include <utility>

/** Interface for delegates with a specific set of arguments **/
template<typename... args>
class AbstractDelegate
{
  public:
    virtual void operator()(args&&...) const = 0;
    virtual ~AbstractDelegate() {}
};

/** Concrete function delegate that discards the function's return value **/
template<typename ReturnType, typename... args>
class FnDelegate : public AbstractDelegate<args...>
{
  public:
    /** member function typedef **/
    using Fn = ReturnType(*)(args...);

    /** constructor **/
    FnDelegate(Fn fn)
      : fn_{fn}
    {
    }

    /** call operator that calls the stored function **/
    void operator()(args&&... a) const override
    {
      (*fn_)(std::forward<args>(a)...);
    }

  private:
    /** function pointer **/
    const Fn fn_;
};

/** forward declaration **/
template<typename... args>
class Connection;

/** Signal class that can be connected to**/
template<typename... args>
class Signal
{
  public:
    /** connection pointer typedef **/
    typedef Connection<args...>* connection_p;

    /** constructor **/
    Signal()
      : connections_(NULL),
      blocked_(false)
      {
      }

    /** call operator that notifes all connections associated with this Signal.
      The most recently associated connection will be notified first **/
    void operator()(args&&... a) const
    {
      // only notify connections if this signal is not blocked
      if (!blocked())
      {
        auto c = connections_;
        while(c != NULL)
        {
          (*c)(std::forward<args>(a)...);
          c = c->next();
        }
      }
    }

    /** connect to this signal **/
    void connect(connection_p p)
    {
      p->next_ = connections_;
      connections_ = p;
      p->signal_ = this;
    }

    /** disconnect from this signal.
      Invalidates the connection's signal pointer
      and removes the connection from the list **/
    void disconnect(connection_p conn)
    {
      // find connection and remove it from the list
      connection_p c = connections_;
      if (c == conn)
      {
        connections_ = connections_->next();
        conn->next_ = NULL;
        conn->signal_ = NULL;
        return;
      }
      while(c != NULL)
      {
        if (c->next() == conn)
        {
          c->next_ = conn->next();
          conn->next_ = NULL;
          conn->signal_ = NULL;
          return;
        }
        c = c->next();
      }
    }

    /** block events from this signal **/
    void block()
    {
      blocked_ = true;
    }

    /** unblock events from this signal **/
    void unblock()
    {
      blocked_ = false;
    }

    /** is this signal blocked? **/
    bool blocked() const
    {
      return blocked_;
    }

    /** destructor. disconnects all connections **/
    ~Signal()
    {
      connection_p p = connections_;
      while(p != NULL)
      {
        connection_p n = p->next();
        disconnect(p);
        p = n;
      }
    }

    connection_p connections() const {return connections_;}

  private:
    connection_p connections_;
    bool blocked_;
};

/** connection class that can be connected to a signal **/
template<typename... args>
class Connection
{
  public:
    /** template constructor for static member functions and free functions.
      allocates a new delegate on the heap **/
    template<typename ReturnType>
    Connection(Signal<args...>& signal, ReturnType (*Fn)(args...))
      : delegate_(new FnDelegate<ReturnType, args...>(Fn)),
      signal_(NULL),
      next_(NULL),
      blocked_(false)
    {
      signal.connect(this);
    }

    /** get reference to this connection's delegate **/
    AbstractDelegate<args...>& delegate() const
    {
      return *delegate_;
    }

    /** call this connection's delegate if not blocked **/
    void operator()(args&&... a) const
    {
      if (!blocked())
      {
        delegate()(std::forward<args>(a)...);
      }
    }

    /** get pointer to next connection in the signal's list **/
    Connection* next() const
    {
      return next_;
    }

    /** is this connection connected to a valid signal? **/
    bool connected() const
    {
      return (signal_ != NULL);
    }

    /** block events for this connection **/
    void block()
    {
      blocked_ = true;
    }

    /** unblock events for this connection **/
    void unblock()
    {
      blocked_ = false;
    }

    /** is this connection blocked? **/
    bool blocked() const
    {
      return blocked_;
    }

    /** desctructor. If the signal is still alive, disconnects from it **/
    ~Connection()
    {
      if (signal_ != NULL)
      {
        signal_->disconnect(this);
      }
      delete delegate_;
    }

    const Signal<args...>* signal() const {return signal_;}

    friend class Signal<args...>;
  private:
    AbstractDelegate<args...>* delegate_;
    Signal<args...>* signal_;
    Connection* next_;
    bool blocked_;
};

/** free connect function: creates a connection (static member or free function) on the heap
  that can be used anonymously **/
template<typename ReturnType, typename... args>
Connection<args...>* connect(Signal<args...>& signal, ReturnType (*fn)(args...))
{
  return new Connection<args...>(signal, fn);
}

#endif // _SIGNALS_H_

I'm trying to use it in these ways:

Signal<int> sig;

void print(int i)
{
  std::cout << "print(" << i << ")" << std::endl;
}

int get(int i)
{
  return i;
}

int main()
{
  connect(sig, print);
  sig(3);
  int i = 4;
  sig(i); // <-- here I get an error
  sig(get(5));
}

The error I get is

main.cpp: In function ‘int main()’:
main.cpp:21:10: error: cannot bind ‘int’ lvalue to ‘int&&’
 sig(i);
      ^
In file included from main.cpp:2:0:
main.h:89:10: error:   initializing argument 1 of ‘void Signal<args>::operator()(args&& ...) const [with args = {int}]’
 void operator()(args&&... a) const
      ^

The errors vanishes when I use const int& everywhere, i.e. Signal<const int&> sig and void print(const int&), but I don't understand why. Also, it would feel awkward to pass around a const bool& in case of "flag" signals.

Can you suggest a fix that would allow for some more flexibility here?

Answer 1

Your code review was a failure. The universal references technique is not naively applicable to virtual abstract interfaces.

Do not use Args&& in the signature, use Args..., of your pure virtual interface. Inside the implementation, the last time (or only time) you use the arg use std::forward<Args>(args)... to conditionally move if and only if the argument type is a && rvalue reference or a literal.

When you use Args&&..., your function takes either lvalue references or rvalue references. You want neither in many cases where you are explicitly passing the arguement types in. In a type deduction context Args&&... instead auto-detects the 'best' match for your argument types, but you are not deducing these types.

When you take Args... in a type deduced context, the type deduced is always literals. Often this is sub optimal. But if you are specifying the types, there is no problem.

The use of std::forward conditionally moves the variables. It moves if the type passed in is an rvalue reference or is a literal. This happens to just do the right thing when both perfect forwarding universal references, and in the use case I describe above.

template<typename... args>
class AbstractDelegate
{
  public:
    virtual void operator()(args...) const = 0;
    virtual ~AbstractDelegate() {}
};

/** Concrete function delegate that discards the function's return value **/
template<typename ReturnType, typename... args>
class FnDelegate : public AbstractDelegate<args...>
{
  public:
    /** member function typedef **/
    using Fn = ReturnType(*)(args...);

    /** constructor **/
    FnDelegate(Fn fn)
      : fn_{fn}
    {
    }

    /** call operator that calls the stored function **/
    void operator()(args... a) const override
    {
      (*fn_)(std::forward<args>(a)...); // last (&only) use of each arg, ok to forward
    }

...

In Signal, you can either remove forward (keep && as it is a non-virtual interface), or you can do something more complex and only forward on the last signal:

// stay args&&... here:
void operator()(args&&... a) const {
  // only notify connections if this signal is not blocked
  if (!blocked())
  {
    auto c = connections_;
    while(c)
    {
      auto c_next = c->next();
      if (c_next)
        (*c)(a...);
      else
        (*c)(std::forward<args>(a)...); // last use, can forward
      c = c_next;
    }
  }
}

As forward is a conditional move, for the most part you should only use it in a context where move would be valid. (Not quite, of course, due to the conditional nature of the move, but the pattern is valid).

As it is not valid to move from a variable more than once, it is not valid to forward from a variable more than once.

Answer 2

Walking this through from the eyes of the compiler, the following simplified extract of your code is the root problem:

template <typename... Args>               // (a)
struct Signal
{
    void operator()(Args&&... a) const;   // (b)
};

int main()
{
    Signal<int> sig;                      // (c)
    int i = 4;                            // (d)
    sig(i);                               // (e)
}

Line (c) instantiates Signal<int>, which via lines (a,b) has a method void operator()(int&& a) const;. Line (d) instantiates a named variable of type int. Line (e) attempts to pass that int to a method that requires a value of type int&&. The compiler correctly complains about this.

At this point you should already be able to explain why replacing your templated types with const int& doesn't have the same problem: since there is not Args&& expansion, there are no r-value references being declared. The regular reference to const l-values work as you expect.

One way to correct the reported error would be to cast the int to an int&&; this sort of cast allows passing it to the called function to invalidate its contents. To make it stand out in the code, this operation is done via std::move(i):

    sig(std::move(i));                    // (e')

While this would address the compiler error, this is not really what you want. Yakk's comments about the interaction between perfect forwarding and virtual function inheritance is really a comment on templated functions and inheritance. And it's correct, except, as Yakk notes, Signal<Args> isn't an inherited class. (It becomes relevant in the AbstractDelegate class tree, which is in an inherited class where all these problems are repeated, but let's set that aside for the moment.) What you really want here is merely to omit the && on line (b), and drop the use of std::forward<>():

    void operator()(Args... a) const;     // (b')

And that's where you should probably stop for now. Once you're ready to dive deeper into what went wrong with the suggestions of the code review, continue on.

---

To understand the problems, the next step is to examine why one might use perfect forwarding. To understand that, you'll also have to understand move semantics, which in turn means understanding r-values and r-value references. There's a lot of good literature out there on this, but the summary version of this is as follows.

In some scenarios, similar to your scenario here, one function tries to expose the same interface as another function, perhaps plus or minus a parameter. In C++03, you had to know at least the number of parameters so that you could create enough template parameters to match them. In C++11, you can use variadic templates to automatically match both the count and type of parameters. However C++11 also introduces the concept of moving values. Moving a value is typically more efficient than constructing a copy, so it's beneficial to support this. Functions accept moved values by accepting r-value references, for example int&&. (In practice int is a bad example, as it's trivial to copy; std::vector<int> would be a more relevant example. But I'll stick with int for simplicity.)

However, r-value references are weird. When you declare a parameter of int&&, the local variable in the function has a different type: plaint int. So even a function parameter that accepts a moved variable (a parameter of type r-value reference -- T&&) declares a local l-value and cannot be implicitly moved. Perfect forwarding is the concept of knowing the parameter was an r-value reference and using this information to, in-turn, move the parameter's value into the function it calls. Syntactically this looks like std::forward<int&&>(i), but since you'd write that as std::move(i) if you knew it was supposed to be moved, std::forward is only actually useful in template functions. Thus it really looks more like std::forward<T>(i) or, in the case of a variadic pack, std::forward<Args>(a)....

Should your code use perfect forwarding? I would say probably not. To decide, first you have to determine whether this part of the code is or is likely to be performance critical. If you're not writing a generic library and you don't have profile data that says otherwise, you should assume it is not performance critical. Secondly, Yakk's point about templated virtual functions comes back; you cannot override a virtual function of a different type, so the templates are not useful with the level of type erasure provided by your AbstractDelegate class. Since you cannot forward perfectly there, there's minimal benefit to forwarding on the level that calls it. There's even a decent chance that the compiler optimizations will effectively omit the outer layer, going directly to the virtual function call anyway.

Let's say you ignore this advice, and decide you want perfect forwarding anyway. To get perfect forwarding, you need to be in a location where the compiler is performing template argument deduction. For that you would need to replace line (b) similarly:

    template <typename... OpArgs>
    void operator()(OpArgs&&... a) const; // (b'')

This way each deduced value with the OpArgs pack will reflect the l-value or r-value nature of the parameter passed to the method. Your call on line (e) will generate a function void Signal<Args>::operator()(int a) const; which will accept the int it passes. This is fine in the Signal class, but will result in problems overriding virtual functions in AbstractDelegate's inherited types. Why? Because AbstractDelegate<Args> will only create one virtual function to be overridden, but each different instantiation of operator() could require a different virtual function. This isn't a problem with line (b) as originally written, as the templated types are specified at the class level, and thus there is exactly one specification of the function. But that blocks the ability to perfectly forward.

Per aschepler's comment, using std::function<void(args...)> instead of your custom AbstractDelegate class tree simplifies your code, but does not change the above. You'll note that std::function does not perfectly forward.

And that's why I'd recommend not bothering with perfect forwarding here. It was a red herring in the first place, and it led you down a bad path in this particular case.