On Tue, Aug 09, 2016 at 11:51:18AM +0200, Giulio Camuffo wrote: > 2016-08-09 10:17 GMT+02:00 Jonas Ådahl <jad...@gmail.com>: > > On Tue, Jul 05, 2016 at 09:51:10AM +0200, Giulio Camuffo wrote: > >> To complement on the new resource created signal, this allows to > >> iterate over the existing resources of a client. > >> > >> Signed-off-by: Giulio Camuffo <giulio.camu...@kdab.com> > > > > I know the break:ability of the iteration was discussed in the previous > > version. I can't think of a situation where it'll ever break. If you > > need to iterate through all the resources to get a particular one, you > > are probably doing something wrong. It also makes the API a bit more > > confusing, since a numeric return value that effects a for-each loop > > will always be hard to follow. > > Say that a tool has a list view to show the resources, it may want to > break the loop when the visible part of the list is full. > > > > > FWIW, glib tries to solve a similiar API issue by trying to make people > > return G_SOURCE_CONTINUE and G_SOURCE_REMOVE for a similar situation, > > because returning FALSE/TRUE made reading the code annoying, but without > > actually using a enum or something like that forcing people to write > > more readable code, it's almost pointless. > > I'm not sure i understand. Are you saying using an enum would be a > good or bad idea? > An alternative solution could be to keep a void return type, and to > pass a "bool *continue" to the iterator, initially > initialized to true, which the iterator sets to false when it wants to > break the loop. So in the easy case the iterator doesn't > need to care about it.
I'd say returning an enum is better than returning an int. Passing around a bool pointer is fine too i guess. Both are easier to read than just a special int return value. > > > > > > With that said, the code looks correct to me, and with a couple of > > comments inline, this is: > > > > Reviewed-by: Jonas Ådahl <jad...@gmail.com> > > > >> --- > >> > >> v2: - don't cast the function pointer, but make it a two step process > >> - make it possible to break the loop early > >> > >> src/wayland-private.h | 2 +- > >> src/wayland-server-core.h | 8 ++++++++ > >> src/wayland-server.c | 47 > >> ++++++++++++++++++++++++++++++++++++++++++++++- > >> src/wayland-util.c | 8 ++++++-- > >> 4 files changed, 61 insertions(+), 4 deletions(-) > >> > >> diff --git a/src/wayland-private.h b/src/wayland-private.h > >> index 045109b..b715534 100644 > >> --- a/src/wayland-private.h > >> +++ b/src/wayland-private.h > >> @@ -74,7 +74,7 @@ struct wl_map { > >> uint32_t free_list; > >> }; > >> > >> -typedef void (*wl_iterator_func_t)(void *element, void *data); > >> +typedef int (*wl_iterator_func_t)(void *element, void *data); > > > > Not sure if this matters, but this changes the type signature of an > > exported function symbol: wl_map_for_each. While WL_EXPORT:ed, it's not > > exposed via any installed header file, so I suppose it won't cause any > > real problems. > > Oh, indeed... i wonder why it's exported. But i don't think we care, > our API is defined by the headers, right? I suspect it's from when things were moving between wayland and weston/wayland-demos. Could we maybe even unexport it? Would anything break? Jonas > > > Thanks, > Giulio > > > > >> > >> void > >> wl_map_init(struct wl_map *map, uint32_t side); > >> diff --git a/src/wayland-server-core.h b/src/wayland-server-core.h > >> index bb0a989..647d97c 100644 > >> --- a/src/wayland-server-core.h > >> +++ b/src/wayland-server-core.h > >> @@ -216,6 +216,14 @@ void > >> wl_client_add_resource_created_listener(struct wl_client *client, > >> struct wl_listener *listener); > >> > >> +typedef int (*wl_client_for_each_resource_iterator_func_t)(struct > >> wl_resource *, > > > > nit: missing parameter name > > > >> + void > >> *user_data); > >> + > >> +void > >> +wl_client_for_each_resource(struct wl_client *client, > >> + wl_client_for_each_resource_iterator_func_t > >> iterator, > >> + void *user_data); > >> + > >> /** \class wl_listener > >> * > >> * \brief A single listener for Wayland signals > >> diff --git a/src/wayland-server.c b/src/wayland-server.c > >> index 067d8a5..98e3bda 100644 > >> --- a/src/wayland-server.c > >> +++ b/src/wayland-server.c > >> @@ -565,7 +565,7 @@ wl_resource_post_no_memory(struct wl_resource > >> *resource) > >> WL_DISPLAY_ERROR_NO_MEMORY, "no memory"); > >> } > >> > >> -static void > >> +static int > >> destroy_resource(void *element, void *data) > >> { > >> struct wl_resource *resource = element; > >> @@ -580,6 +580,8 @@ destroy_resource(void *element, void *data) > >> > >> if (!(flags & WL_MAP_ENTRY_LEGACY)) > >> free(resource); > >> + > >> + return 0; > >> } > >> > >> WL_EXPORT void > >> @@ -1603,6 +1605,49 @@ wl_client_add_resource_created_listener(struct > >> wl_client *client, > >> wl_signal_add(&client->resource_created_signal, listener); > >> } > >> > >> +struct wl_resource_iterator_context { > >> + void *user_data; > >> + wl_client_for_each_resource_iterator_func_t it; > >> +}; > >> + > >> +static int > >> +resource_iterator_helper(void *res, void *user_data) > >> +{ > >> + struct wl_resource_iterator_context *context = user_data; > >> + struct wl_resource *resource = res; > >> + return context->it(resource, context->user_data); > >> +} > >> + > >> +/** Iterate over all the resources of a client > >> + * > >> + * \param client The client object > >> + * \param iterator The iterator function > >> + * \param user_data The user data pointer > >> + * > >> + * The function pointed by \a iterator will be called for each > >> + * resource owned by the client. The \a user_data will be passed > >> + * as the second argument of the iterator function. > >> + * If the \a iterator function returns a value less than 0 the loop > >> + * will stop. > > > > nit: "... the iteration will stop." > > > > > > Jonas > > > >> + * > >> + * Creating and destroying resources while iterating is safe, but new > >> + * resources may or may not be picked up by the iterator. > >> + * > >> + * \memberof wl_client > >> + */ > >> +WL_EXPORT void > >> +wl_client_for_each_resource(struct wl_client *client, > >> + wl_client_for_each_resource_iterator_func_t > >> iterator, > >> + void *user_data) > >> +{ > >> + struct wl_resource_iterator_context context = { > >> + .user_data = user_data, > >> + .it = iterator, > >> + }; > >> + > >> + wl_map_for_each(&client->objects, resource_iterator_helper, > >> &context); > >> +} > >> + > >> /** \cond */ /* Deprecated functions below. */ > >> > >> uint32_t > >> diff --git a/src/wayland-util.c b/src/wayland-util.c > >> index 7467366..856e53b 100644 > >> --- a/src/wayland-util.c > >> +++ b/src/wayland-util.c > >> @@ -363,13 +363,17 @@ static void > >> for_each_helper(struct wl_array *entries, wl_iterator_func_t func, void > >> *data) > >> { > >> union map_entry *start, *end, *p; > >> + int ret; > >> > >> start = entries->data; > >> end = (union map_entry *) ((char *) entries->data + entries->size); > >> > >> for (p = start; p < end; p++) > >> - if (p->data && !map_entry_is_free(*p)) > >> - func(map_entry_get_data(*p), data); > >> + if (p->data && !map_entry_is_free(*p)) { > >> + ret = func(map_entry_get_data(*p), data); > >> + if (ret < 0) > >> + break; > >> + } > >> } > >> > >> WL_EXPORT void > >> -- > >> 2.9.0 > >> > >> _______________________________________________ > >> wayland-devel mailing list > >> wayland-devel@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/wayland-devel _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel