On 2018/July/13 05:40, Simon Ser wrote: > This new function allows listeners to remove themselves or any > other listener when called. This version only works if listeners > are properly cleaned up when the wl_signal is free'd. This is about free()ing the wl_listener in the actual handler, not the wl_signal, I think the wording here (and in the code-comment in the patch) is off.
Also I'd say that the commit message needs to be a bit longer. While "we" (anyone who participated in that earlier thread) know what this is about and why we need it, someone who looks at this patch and at wl_signal_emit which uses wl_list_for_each*_safe* will be confused bout what's going on. Ah, I just saw it's in the comment of the function, I'd still move that to the doxygen comment, or commit message and out of the pure code comment > > Signed-off-by: Simon Ser <cont...@emersion.fr> > --- > This is a [1] follow-up. Since we noticed the previous version is > not a drop-in replacement for wl_signal_emit, this patch just adds > a new function instead. > > [1]: https://patchwork.freedesktop.org/patch/204641/ > > src/wayland-server-core.h | 3 ++ > src/wayland-server.c | 47 +++++++++++++++++++++ > tests/signal-test.c | 86 +++++++++++++++++++++++++++++++++++++++ > 3 files changed, 136 insertions(+) > > diff --git a/src/wayland-server-core.h b/src/wayland-server-core.h > index 2e725d9..4a2948a 100644 > --- a/src/wayland-server-core.h > +++ b/src/wayland-server-core.h > @@ -468,6 +468,9 @@ wl_signal_emit(struct wl_signal *signal, void *data) > l->notify(l, data); > } > > +void > +wl_signal_emit_safe(struct wl_signal *signal, void *data); > + > typedef void (*wl_resource_destroy_func_t)(struct wl_resource *resource); > > /* > diff --git a/src/wayland-server.c b/src/wayland-server.c > index eae8d2e..a5f3735 100644 > --- a/src/wayland-server.c > +++ b/src/wayland-server.c > @@ -1932,6 +1932,53 @@ wl_client_for_each_resource(struct wl_client *client, > wl_map_for_each(&client->objects, resource_iterator_helper, &context); > } > > +static void > +handle_noop(struct wl_listener *listener, void *data) { > + /* Do nothing */ > +} > + > +/** Emits this signal, safe against removal of any listener. > + * > + * \note This function can only be used if listeners are properly removed > when > + * the wl_signal is free'd. > + * > + * \param signal The signal object that will emit the signal > + * \param data The data that will be emitted with the signal > + * > + * \sa wl_signal_emit() > + * > + * \memberof wl_signal > + */ > +WL_EXPORT void > +wl_signal_emit_safe(struct wl_signal *signal, void *data) { > + struct wl_listener cursor; > + struct wl_listener end; Semi-serious: We could also add something like: if (signal.listener_list.prev == signal.listener_list.next) { wl_signal_emit(signal); return; } And get rid of that wl_priv_signal, since that supported exactly this little corner-case (as I showed with the additional test a while ago) and not the general asumption that it's safe not to remove a destroy listener in general. > + > + /* Add two special markers: one cursor and one end marker. This way, we > know > + * that we've already called listeners on the left of the cursor and > that we > + * don't want to call listeners on the right of the end marker. The 'it' > + * function can remove any element it wants from the list without > troubles. > + * wl_list_for_each_safe tries to be safe but it fails: it works fine > + * if the current item is removed, but not if the next one is. */ > + wl_list_insert(&signal->listener_list, &cursor.link); > + cursor.notify = handle_noop; > + wl_list_insert(signal->listener_list.prev, &end.link); > + end.notify = handle_noop; > + > + while (cursor.link.next != &end.link) { > + struct wl_list *pos = cursor.link.next; > + struct wl_listener *l = wl_container_of(pos, l, link); > + > + wl_list_remove(&cursor.link); > + wl_list_insert(pos, &cursor.link); > + > + l->notify(l, data); > + } > + > + wl_list_remove(&cursor.link); > + wl_list_remove(&end.link); > +} > + > /** \cond INTERNAL */ > > /** Initialize a wl_priv_signal object > diff --git a/tests/signal-test.c b/tests/signal-test.c > index 7bbaa9f..dc762a4 100644 > --- a/tests/signal-test.c > +++ b/tests/signal-test.c > @@ -115,3 +115,89 @@ TEST(signal_emit_to_more_listeners) > > assert(3 * counter == count); > } > + > +struct signal > +{ > + struct wl_signal signal; > + struct wl_listener l1, l2, l3; > + int count; > + struct wl_listener *current; > +}; > + > +static void notify_remove(struct wl_listener *l, void *data) > +{ > + struct signal *sig = data; > + wl_list_remove(&sig->current->link); > + wl_list_init(&sig->current->link); > + sig->count++; > +} > + > +#define INIT \ > + wl_signal_init(&signal.signal); \ > + wl_list_init(&signal.l1.link); \ > + wl_list_init(&signal.l2.link); \ > + wl_list_init(&signal.l3.link); > +#define CHECK_EMIT(expected) \ > + signal.count = 0; \ > + wl_signal_emit_safe(&signal.signal, &signal); \ > + assert(signal.count == expected); > + > +TEST(signal_remove_listener) > +{ > + test_set_timeout(4); > + > + struct signal signal; > + > + signal.l1.notify = notify_remove; > + signal.l2.notify = notify_remove; > + signal.l3.notify = notify_remove; > + > + INIT > + wl_signal_add(&signal.signal, &signal.l1); > + > + signal.current = &signal.l1; > + CHECK_EMIT(1) > + CHECK_EMIT(0) > + > + INIT > + wl_signal_add(&signal.signal, &signal.l1); > + wl_signal_add(&signal.signal, &signal.l2); > + > + CHECK_EMIT(2) > + CHECK_EMIT(1) > + > + INIT > + wl_signal_add(&signal.signal, &signal.l1); > + wl_signal_add(&signal.signal, &signal.l2); > + > + signal.current = &signal.l2; > + CHECK_EMIT(1) > + CHECK_EMIT(1) > + > + INIT > + wl_signal_add(&signal.signal, &signal.l1); > + wl_signal_add(&signal.signal, &signal.l2); > + wl_signal_add(&signal.signal, &signal.l3); > + > + signal.current = &signal.l1; > + CHECK_EMIT(3) > + CHECK_EMIT(2) > + > + INIT > + wl_signal_add(&signal.signal, &signal.l1); > + wl_signal_add(&signal.signal, &signal.l2); > + wl_signal_add(&signal.signal, &signal.l3); > + > + signal.current = &signal.l2; > + CHECK_EMIT(2) > + CHECK_EMIT(2) > + > + INIT > + wl_signal_add(&signal.signal, &signal.l1); > + wl_signal_add(&signal.signal, &signal.l2); > + wl_signal_add(&signal.signal, &signal.l3); > + > + signal.current = &signal.l3; > + CHECK_EMIT(2) > + CHECK_EMIT(2) > +} > -- > 2.18.0 > > > _______________________________________________ > wayland-devel mailing list > wayland-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/wayland-devel
signature.asc
Description: PGP signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel