fr00b0 / nod

Small, header only signals and slots C++11 library.
MIT License
253 stars 49 forks source link

signal_type add a rvalue operator #4

Closed qicosmos closed 7 years ago

qicosmos commented 8 years ago

hi, the signal_type's opertator is just surpport lvalue, it need a rvalue version, like this.

void operator()(A&&... args) const {
    for (auto const& slot : copy_slots()) {
        if (slot) {
                  slot(std::forward<A>(args)...);
            }
    }
}
fr00b0 commented 8 years ago

Hello, I'm currently looking into this, but I'm not convinced that it's a good idea to implement. Doing a std::forward in a loop like that will open up some cans of worms.

If I were to add the implementation to the library, the code below will fail catastrophically since the first slot will consume the argument:

auto lambda =
    [&]( std::unique_ptr<int> value ) {
        std::cout << *value << std::endl;
    };

nod::signal<void(std::unique_ptr<int>&&)> signal;
signal.connect( lambda );
signal.connect( lambda );
signal( std::make_unique<int>( 5 ) );

When I get some more time to sit down, I'll look into this some more.

If you don't mind me asking, what are the situation where you need the rvalue version?

qicosmos commented 8 years ago

hello, it is meaningless to use rvalue in loop, i just make a unit test, and the code can't be compiled. this is the code:

    nod::signal<void(string&&)> signal;
    signal.connect([](string&& i){});
    signal(string(""));

so i have to add a rvalue version.

fr00b0 commented 8 years ago

Yes I understand that your code will not compile. I'm curious about the situation where you need a signal with a rvalue argument. Will you move arguments into your signals? What happens if that signal has multiple slots connected when it is triggered?

qicosmos commented 8 years ago

I konw your meaning. a rvalue argument in this situation is meaningless. I think you could give some advice to user, maybe somebody makes a mistake.

fr00b0 commented 7 years ago

I'm closing this issue as it's not implementable, but the issue lives on in issue #12