Severe potential bug

[Philippe91]

Run the following code: the assert will fail. This is simply because the lambda is not copied to 'sig', hence its data destructed before sig(10); is reached.

By using std::move(lambda), the problem is solved. But it's easy to forget this and have this bug during run time.

{
	fteng::signal<void(int)> sig;
	struct foo
	{
		~foo() { x = 999; }
		int x = 5;
	};
	int result = 0;
	{
		foo value;
		auto lambda = [&result, value](int count) { result = value.x + count; };
		sig.connect(lambda);
		//sig.connect(std::move(lambda));
	}

	sig(10);

	assert(result == 15);
}

[TheWisp]

Thank you for bringing up the topic, it is actually a more complex design problem than it seems. See an opposite bug, which was sorted out earlier: https://github.com/TheWisp/signals/issues/9 The danger of always copying a functor into the signal is not just about performance; It may subtly affect the correctness and confuse the user about the semantic of connecting-signal-to-functor. Generally, I believe managing the object's lifetime is easier to do than overriding the copying behavior. Also, it is widely accepted that mentioning an l-value means by reference (such as, if you have an int x;, an expression x or especially (x) would refer to the variable, rather than a temporary copy). On the other hand, an r-value returned from an expression or explicit moving is considered to be temporary and always either moved or copied into the destination, hence my earlier design.

I would like more feedback from you!

[Philippe91]

I have added this compile-time assert to connect() to be on the safe side and keep best performances:

static_assert(! std::is_lvalue_reference<F>::value, "Use std::move() or construct{} the functor at call's location");

[mikezackles]

Sorry, I would have commented earlier, but I just noticed the activity here.

I would argue that silently storing references is usually nonintuitive behavior and that copying lvalues is more idiomatic (e.g., vector::push_back). Storing references invites dangling as in the motivating example, which looks like it results in a use after free.

If lvalues are copied, then given the example in #9, I think sig.connect(std::ref(s)) should yield the desired result without any need to modify the functor, and this makes it explicit that the user wants to store a reference (see https://en.cppreference.com/w/cpp/utility/functional/reference_wrapper/operator()).

Furthermore, the existing behavior means that callables that are both movable and copyable can never be copied into connect and must move instead. So it seems to me that copying lvalues is strictly more flexible than storing references.

Just my two cents :)

[dumblob]

Isn't this fixed already? I've skimmed latest commits and it seems addressed there.

[mikezackles]

I do not believe this has been fixed, but you can try my pull request above (#21). The last commit I see was a year ago -- where did you see it was fixed?

[dumblob]

My bad - I've read the commit comment incorrectly and now I've looked at the code and it seems unrelated.