Ok, I see dangling references happening in two ways: (1) not so bright library user doesn't realize that the connection set should be associated with an object and creates a global connection set somewhere. I know that a library doesn't necessarily need to protect users from themselves, but having the container in the public interface for a library is just asking for trouble, at least without clear documentation.

(2) ConnectionSet only supports Add() and Clear(). Obvious problem: User wants to change an Event<> callback to a different member function of the object, forgets to call Clear(). End result: ConnectionSet is now keeping alive two delegates into the Event<>. Again, protecting user from themselves problem, but it's still something I can see happening.

Related problem: User wants to only modify one Event<> attached to a delegate. Needs to flush and rebuild all the connections. Not a big issue, but does result in some unnecessary memory juggling and some cache unhappiness, which doesn't exactly play to the "Very, very fast event library" goal.

Basically, without a clear goal for the ConnectionSet, it just seems to add potential problems without adding any value.

Just thinking aloud here: maybe if you modified it so that a ConnectionSet associated Connection objects with Event<> objects?

Quote:Original post by SiCrane
Ok, I see dangling references happening in two ways: (1) not so bright library user doesn't realize that the connection set should be associated with an object and creates a global connection set somewhere. I know that a library doesn't necessarily need to protect users from themselves, but having the container in the public interface for a library is just asking for trouble, at least without clear documentation.

(2) ConnectionSet only supports Add() and Clear(). Obvious problem: User wants to change an Event<> callback to a different member function of the object, forgets to call Clear(). End result: ConnectionSet is now keeping alive two delegates into theEvent<>. Again, protecting user from themselves problem, but it's still something I can see happening.

You're right, of course. One alternative I had thought about was making ConnectionSet into something intended to be inherited from, a little akin to boost::signals::trackable, with Add() renamed to something a little more specific. I can't decide whether that method would be protected or public, though, and I can think of problems with both approaches.

Quote:Related problem: User wants to only modify one Event<> attached to a delegate. Needs to flush and rebuild all the connections. Not a big issue, but does result in some unnecessary memory juggling and some cache unhappiness, which doesn't exactly play to the "Very, very fast event library" goal.

For this situation, I would envision the object holding a separate Connection, or even a separate ConnectionSet. Of course, this doesn't require the existence of ConnectionSet as opposed to simply a vector<Connection>.

Quote:Basically, without a clear goal for the ConnectionSet, it just seems to add potential problems without adding any value.

Yeh, I'm coming around to your point of view. If/when I figure out how to put in the no-heap-allocation stuff I mentioned earlier, I'll take another look at it, but for now it doesn't fill a need.

Quote:Just thinking aloud here: maybe if you modified it so that a ConnectionSet associated Connection objects with Event<> objects?

What do you mean by this? My design was intended to make Connection as opaque a class as possible, for simplicity and because spangling connections with features is one of the primary reasons for boost::signal's slowness. Were you thinking of a ConnectionSet as a means of recovering this hidden information?

I was just trying to come up with a point for ConnectionSet. It seems like when you add a Connection to the set, you know the Event, the object being bound and the member function. The object being bound should always be the object owning ConnectionSet (or they should be members of the same object, etc.) so that information doesn't need to be kept around. But if you kept a pointer to the Event, and then the ConnectionSet could use the ref counts that the Connections being stored keep, then you could potentially use the ConnectionSet to determine if a given Event<> is still live.

I suppose even without keeping the Event<> pointers around, you can use the ref count information to purge connections if the associated events have been destroyed. So ConnectionSet could have a member PurgeDeadConnections() or something similar. That wouldn't even need friend access if Connection had a member like bool IsEventAlive(void) const { return m_node->refCount > 0; } Then there would be at least a little value added over a vector<Connection>.

Still just thinking out loud.

That reminds me. You might want to add a swap() function for Connections that just swaps the node pointers.

The need for a Purge method is something I've thought a lot about. Event purges only on Invoke, of course, which potentially increases memory use, but I'm hard-pressed to invent a situation where that would be both significant and long-term. As for connections referring to dead events, I don't think it's that much of a problem: in how people use the Observer pattern that I've seen, Observers rarely outlive their Subjects by long.

I see what you mean about ConnectionSet becoming a more featureful event-management system. I have no need for such a thing, at least that I'm yet aware of, so I probably can't design a good one.

Quote:You might want to add a swap() function for Connections that just swaps the node pointers.

I'll specialize std::swap for that.

I can come up with situations where an observer would significantly outlive the subject but not in C++ code you'd actually see in a released product. For example, in C# someone might dynamically create a form and then associate a delegate with long lived object to an OnClick() for a button, and then destroy the form after its processing is done. However, in similar situations in C++, programmers just generally keep a single copy of a dialog around since C++ programmers tend to be more allocation adverse. Even in the situations I can imagine, I don't see the need for a ConnectionSet::Purge(), since none of those situations need a full ConnectionSet, just a single Connection object.

Quote:
I'll specialize std::swap for that.

Also consider adding a version in your events namespace. Koenig lookup is your friend. Except when it isn't.

Quote:Original post by SiCrane
Koenig lookup is your friend. Except when it isn't.

Wouldn't this be an "isn't" situation? If I put it only in events, std::swap will be transparently inefficient. And if I put it in both, using namespace std or events will cause an ambiguity.

Alright, I've had an idea. *cue ominous thunder* I've added an AutoTracked class, very similar to boost::signals::trackable. To use it, you inherit from it, and then instead of calling Register() you call AutoRegister() (not wild about the name, but whatever), a function which puts the connection in a private ConnectionSet (a class which has finally found a use, though it may move to detail) and returns void. So the basic usage mode is to only ever call AutoRegister(); but if you need finer-grained control, you can call Register() instead and deal with the Connection yourself.

I've also made some minor changes to allow the code to compile cleanly under /W4.

Quote:Original post by Sneftel

Quote:Original post by SiCrane
Koenig lookup is your friend. Except when it isn't.

Wouldn't this be an "isn't" situation? If I put it only in events, std::swap will be transparently inefficient. And if I put it in both, using namespace std or events will cause an ambiguity.

Putting it in both shouldn't cause an ambiguity. ADL should always pickup the version in the events namespace unless std::swap is explicitly used, even if using namespace std, events or both is in scope.

Regarding the AutoTracked class: I'm not sure that private inheritance is a good idea here. Or, at least, if you use private inheritance, put in a using ConnectionSet::Clear. Or, (and this is really the ickiest of the ideas) add functionality for the AutoTracked class to detect that its been registered with the same Event<> multiple times.

Also, I'd probably move AutoRegister to become a member function of AutoTracked rather than AutoRegister. At time of call of AutoRegister, you need to know three things: The identity of the object, the identity of the member function and the identity of the event. With AutoRegister in the Event<> class, it's possible to mistakenly call AutoRegister with a non-AutoTracked object; with AutoRegister as part of the AutoTracked class it's not. Though I admit that you took the time out to make the conversion error understandable.