[PATCH wayland v4] util: Document wl_list methods

Tue Sep 20 09:50:06 UTC 2016

On Tue,  6 Sep 2016 06:28:19 -0700
Yong Bakos <junk at humanoriented.com> wrote:

> From: Yong Bakos <ybakos at humanoriented.com>
> 
> Add doxygen comment blocks to all wl_list methods.
> 
> Signed-off-by: Yong Bakos <ybakos at humanoriented.com>
> ---
> v4: Fix variable name in sample code. (pq)
>     Remove implemenetation details of pointer state. (pq)
>     Remove note about __typeof__ entirely.
>     - it's not helpful as a note or a code comment either
>     Change sample code indentation to just spaces. (pq)
>     Use _list suffix for list in sample code. (pq)
>     Use 'iterate' instead of enumerate, for consistency. (pq)
>     Note that only removing 'pos' is safe for *_safe methods. (pq, giucam)
> v3: Standardize on term 'element', to match param names, tests, and other text.
>     Use 'relates' for macros (instead of 'memberof').
> v2: Refine the writing for clarity.
>     Add dox for wl_list macros, omitted in v1.
>     Add notices for unsafe operations and invalid states (giucam, pq)
> 
> 
>  src/wayland-util.h | 221 +++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 181 insertions(+), 40 deletions(-)
> 
> diff --git a/src/wayland-util.h b/src/wayland-util.h
> index cacc122..6d01408 100644
> --- a/src/wayland-util.h
> +++ b/src/wayland-util.h
> @@ -78,73 +78,147 @@ struct wl_interface {
> 
>  /** \class wl_list
>   *
> - * \brief doubly-linked list
> + * \brief Circular doubly-linked list

Hi,

sorry I didn't nitpick about this before. Is "circular" really correct
here?

Yes, if you only look at struct wl_list, then the list is circular.
However, you cannot take an arbitrary list element and then iterate
through all the other elements. One of the wl_list objects in the list
is the list head, which is not contained in the same struct type as
the real member elements. Dereferencing the head element as a member
element is a bug.

You probably could construct a truly circular list that has no head
element, but its use would be fairly different to the usual case. The
head would (preferably) not be a struct wl_list since it cannot work as
the head in any of the wl_list_*() API. One can make it work perhaps,
but I have never seen an example of it, except in rare destruction cases
where you remove the list head and leave all the elements in place, so
that each element can be unconditionally removed later (I'd call that a
"headless list"). In the general case, I think you might have trouble
updating the "head" pointer on element removal.

>   *
> - * The list head is of "struct wl_list" type, and must be initialized
> - * using wl_list_init().  All entries in the list must be of the same
> - * type.  The item type must have a "struct wl_list" member. This
> - * member will be initialized by wl_list_insert(). There is no need to
> - * call wl_list_init() on the individual item. To query if the list is
> - * empty in O(1), use wl_list_empty().
> + * On its own, an instance of `struct wl_list` represents the sentinel head of
> + * a circular, doubly-linked list, and must be initialized using wl_list_init().
> + * When empty, the list head's `next` and `prev` members point to the list head
> + * itself, otherwise `next` references the first element in the list, and `prev`
> + * refers to the last element in the list.
>   *
> - * Let's call the list reference "struct wl_list foo_list", the item type as
> - * "item_t", and the item member as "struct wl_list link".
> + * Use the `struct wl_list` type to represent both the list head and the links
> + * between elements within the list. Use wl_list_empty() to determine if the
> + * list is empty in O(1).
> + *
> + * All elements in the list must be of the same type. The element type must have
> + * a `struct wl_list` member, often named `link` by convention. There is no need
> + * to initialize an element's `link` - invoking wl_list_init() on an individual
> + * list element's `struct wl_list` member is unnecessary.

It is only unnecessary if the very next operation is wl_list_insert().
However, it is a common idiom to wl_list_init() a link, so that it is
always safe to wl_list_remove() it. Otherwise you need another variable
to check whether you need to wl_list_remove() first. I see the original
doc was already a bit vague on this.

> + *
> + * Consider a list reference `struct wl_list foo_list`, an element type as
> + * `struct element`, and an element's link member as `struct wl_list link`.
> + *
> + * The following code initializes a list and adds three elements to it.
>   *
> - * The following code will initialize a list:
>   * \code
>   * struct wl_list foo_list;
>   *
> - * struct item_t {
> - * 	int foo;
> - * 	struct wl_list link;
> + * struct element {
> + *         int foo;
> + *         struct wl_list link;
>   * };
> - * struct item_t item1, item2, item3;
> + * struct element e1, e2, e3;
>   *
>   * wl_list_init(&foo_list);
> - * wl_list_insert(&foo_list, &item1.link);	// Pushes item1 at the head
> - * wl_list_insert(&foo_list, &item2.link);	// Pushes item2 at the head
> - * wl_list_insert(&item2.link, &item3.link);	// Pushes item3 after item2
> + * wl_list_insert(&foo_list, &e1.link);   // e1 is the first element
> + * wl_list_insert(&foo_list, &e2.link);   // e2 is now the first element
> + * wl_list_insert(&e2.link, &e3.link); // insert e3 after e2
>   * \endcode
>   *
> - * The list now looks like [item2, item3, item1]
> + * The list now looks like <em>[e2, e3, e1]</em>.
> + *
> + * The `wl_list` API provides some iterator macros. For example, to iterate
> + * a list in ascending order:
>   *
> - * Iterate the list in ascending order:
>   * \code
> - * item_t *item;
> - * wl_list_for_each(item, foo_list, link) {
> - * 	Do_something_with_item(item);
> + * struct element *e;
> + * wl_list_for_each(e, foo_list, link) {
> + *         do_something_with_element(e);
>   * }
>   * \endcode
> + *
> + * See the documentation of each iterator for details.
> + * \sa http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/list.h
>   */
>  struct wl_list {
>  	struct wl_list *prev;
>  	struct wl_list *next;
>  };

I can't find anything to complain about in the rest, very good! :-)

Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160920/e823b3fc/attachment-0001.sig>