Cannot move std::function into map

Question

I have some Signal prototype class

#include <map>
#include <string>
#include <functional>

template <class T>
class Signal
{
public:
    std::function<T> slot;
};

Next comes template singleton SignalCollection class, that automatically generates appropriate type for Signal

template <class T>
class SignalCollection
{
private:
    SignalCollection() {}
    SignalCollection(const SignalCollection&) = delete;
    SignalCollection& operator= (const SignalCollection&) = delete;
public:

    static SignalCollection& Instance()
    {
        static SignalCollection br{};
        return br;
    }

    std::map<std::string, T> signals_map;

    void add(T&& signal)
    {
        this->signals_map.insert(std::make_pair("a", std::forward<T>(signal)));
    }
};

and last I have a function that deduces SignalCollection type for some Signal

template<class T>
auto& get_collection_for_signal(T&& t)
{
    return SignalCollection<T>::Instance();
}

The problem is, that I can't add values to the map of collection. Here is main:

void foo()
{

}
void main()
{
    Signal<void()> sgl = Signal<void()>{}; //Create signal
    sgl.slot = foo;                       //add slot

    auto& t = get_collection_for_signal(sgl); //Get collection for this signal which is SignalCollection<Signal<void()>>

    t.add(sgl); //error 1
    t.signals_map["a"] = sgl; //error 2
}

Please copy-paste full error messages (from the "Output" tab, instead of the "Error List"), since ones that you show here, doesn't seem complete. — Algirdas Preidžius, Mar 13 '19 at 18:22
1) This is not what I was pointing out. 2) Now `//error 1` `//error 2` comments, doesn't make sense, due to the fact, that your question no longer contains any errors, that you get. — Algirdas Preidžius, Mar 13 '19 at 18:30

NathanOliver · Accepted Answer · 2019-03-13T18:38:41.423

The problem here is

template<class T>
auto& get_collection_for_signal(T&& t)
{
    return SignalCollection<T>::Instance();
}

When you call it like

auto& t = get_collection_for_signal(sgl);

T is deduced as Signal<void()>& and that means you return a SignalCollection<Signal<void()>&> which is not what you want. What you need to do is remove the reference from the type. You can do that using

template<class T>
auto& get_collection_for_signal(T&&)
{
    return SignalCollection<std::remove_cv_t<std::remove_reference_t<T>>>::Instance();
}

which removed the cv qualification and the reference from T (C++20 gives us std::remove_cvref so it can be done with a single helper).

You can also get the same behavior with

template<class T>
auto& get_collection_for_signal(T)
{
    return SignalCollection<T>::Instance();
}

This involves a lot less typing and gives you the same behavior since top level cv qualifications get striped out and it will never deduce a reference.

You also have an issue with your add function.

void add(T&& signal)
{
    this->signals_map.insert(std::make_pair("a", std::forward<T>(signal)));
}

isn't using a forwarding reference since T is part of the class type. You need to make it it's own template to have a forwarding reference. You can do that by changing it to

template<typename U>
void add(U&& signal)
{
    this->signals_map.insert(std::make_pair("a", std::forward<U>(signal)));
}

lastly

void main()

is always wrong. main() is mandated to return an int. Have a read through What should main() return in C and C++? for more information.

score 2 · Answer 2 · answered Mar 13 '19 at 18:29

T&& is not a forwarding reference if T is not deduced, and converse.

template<class T>
auto& get_collection_for_signal(T&& t)

T is deduced as a reference to Signal<void()> in your example use:

auto& t = get_collection_for_signal(sgl);

so it returns:

return SignalCollection<T>::Instance();

which is:

SignalCollection<Signal<void()>&>::Instance()

which is nonsense.

Brush up on forwarding references and l/rvalue references. Every use of it in your code is wrong.

Design wise, you have useless types -- Signal<T> as written is a std function with baggage that does nothing -- global singletons, and type deduction creating global state.

I get what you are trying to do, but it is kin to wanting to make a fire engine out of straw.

score 2 · Answer 3 · answered Mar 13 '19 at 18:35

The other answers are good at explaining the error, but I would recommend another solution to fix the problem.

I see that in your code you don't actually need a forwarding reference. You only use it for deduction. In that case, you can take your argument by T const&, which always will deduce a T without a reference.

template<class T>
auto& get_collection_for_signal(T const& t)
{
    return SignalCollection<T>::Instance();
}

Cannot move std::function into map

3 Answers3