On Wed, Apr 10, 2013 at 3:51 AM, Alexander Drozdov <dzal_m...@mtu-net.ru> wrote: > The idea is good, but why to not just register finalizer callback on > event initialization? A data pointer is passed to event_assign() or > event_new() > so we can pass finalizer along with the data.
Hi, Alexander! Thanks for the feedback. My main reasons were: * Adding an extra function pointer in every event_callback structure[*] seemed like something to avoid. * There's a handy "finalize_many" pattern I wanted to use for bufferevents to avoid having to call 6-8 finalizer functions on every single bufferevent_free()[*]. That function has weird semantics, though, that are less compatible with the . * To me, it seemed more intuitive and easy to understand than the alternative. This way, it is explicit what code will get invoked by event_free_finalize() and event_finalize(). (There's going to be action-at-a-distance either way: either the finalization call doesn't tell you what will happen, or the setup call doesn't tell you how the code will finalize it later.) > With this approach, > - finalizer, if set, will be called automatically after calling > event_free() or event_del() and after event execution event_del() wouldn't be an appropriate time to do that. event_del() doesn't mean we're tearing down a struct event; it means that we're turning it from active and/or pending into non-active, non-pending. > - The event initialization code includes reference to event deinitialization > one, > which makes the code easier to understand That part does sound like a good thing, if the current approach really is hard to understand. (I wonder how others feel about this point?) > - event_assign(), event_new() and event_get_assignment() should be extended > (for compatibility, new functions should be added) Or, if you wanted to do this, a single event_set_finalizer() function seems like it would work fine too. > - event_get_finalizer() should also be added > - event_del_noblock() & event_free_noblock() or event_del_ext() & > event_free_ext() > with extra flags is still needed. One extra flag DONT_FINALIZE is > desirable to > allow user to call finalizer manually. Again, event_del() does not mean that we should finalize an event. Remember, after event_del(), it's legal to call event_add() on the same event again, so we would need to have an explicit event_finalize() call. So, how do other folks feel? Is it really more readable to specify a cleanup function at event initialization time than at event teardown time? Is it worth it? I'll go with something like Alexander's proposal if there seems like a rough consensus that it's better. [*] event_callback is a new feature in the 2.1 backend logic: it unifies "deferred_callback" and "event" by giving them a supertype. If we convert bufferevent and evbuffer callbacks to event_callback too, we can simplify their code significantly, and use common logic to make all the callbacks in libevent get handled more efficiently. best wishes, -- Nick *********************************************************************** To unsubscribe, send an e-mail to majord...@freehaven.net with unsubscribe libevent-users in the body.
