[ccan] Additional macro to iterate over list (#1)

[Rusty Russell]

On Tue, 22 Nov 2011 02:20:27 -0800, ndreys <reply+i-2255063-93fcad02060c514ba3c4dd5329ddee92ebf69400-775844 at reply.github.com> wrote:
> The last commit is completely unrelated. But it was really annoying not to be able to expand macros in gdb and I didn't want to open a separate pull request for such a small change. Feel free to ask me to discard it.

OK, I'll review the combined list_for_each_off() patch here.  Since it's
one of the first patches I've had from you, I'm going to really nitpick
:)

> diff --git a/ccan/list/list.h b/ccan/list/list.h
> index 18f077a..72894f8 100644
> --- a/ccan/list/list.h
> +++ b/ccan/list/list.h
> @@ -1,6 +1,7 @@
>  /* Licensed under LGPLv2.1+ - see LICENSE file for details */
>  #ifndef CCAN_LIST_H
>  #define CCAN_LIST_H
> +#include <stddef.h>
>  #include <stdbool.h>
>  #include <assert.h>
>  #include <ccan/container_of/container_of.h>
> @@ -297,6 +298,45 @@ static inline void list_del_from(struct list_head
> *h, str> uct list_node *n)
>  #define list_tail(h, type, member) \
>  	(list_empty(h) ? NULL : list_entry((h)->n.prev, type, member))
>  
> +
> +#define __ptr_add_off(ptr, off)                 \
> +  ((void *) ((char *) (ptr) + (ptrdiff_t) (off)))
> +#define __ptr_sub_off(ptr, off)                 \
> +  ((void *) ((char *) (ptr) - (ptrdiff_t) (off)))

I wouldn't cast to ptrdiff_t here; it allows them to pass anything as
"off".

I also would avoid the __ prefix; I like it, but reserved-word-pedants
don't.  And you should stick to the "list_" namespace implied.

Finally, I think it's clearer as list_node_from_off_ and list_node_to_off_:

static inline void *list_node_to_off_(struct list_node *node, size_t off)
{
        return (char *)node - off;
}

static inline struct list_node *list_node_from_off_(void *ptr, size_t off)
{
        return (char *)ptr + off;
}

> +/**
> + * list_for_each_off - iterate through a list of memory regions.
> + * @h: the list_head
> + * @i: the pointer to a memory region wich contains list node data.
> + * @off: offset(relative to @i) at which list node data resides.
> + *
> + * This is a low-level wrapper to iterate @i over the entire list, used to
> + * implement all oher, more high-level, for-each constructs. It's a for loop,
> + * so you can break and continue as normal.

Because these are all macros, we can order them any way we like.  Thus I
would put this towards the end of the header file after the "normal"
loops, which will signficantly reduce the description as well.

> + * Example:
> + *	list_for_each_off(&parent->children, child, sizeof(const char *))
> + *		printf("Name: %s\n", child->name);
> + */

Use offsetof() here, instead of sizeof(const char *).  More explicit.

> +#define list_for_each_off(h, i, off)                                    \
> +  for (i = __ptr_sub_off(list_debug(h)->n.next, off);                   \
> +       (struct list_node *) __ptr_add_off(i, off) != &(h)->n;           \
> +       i = __ptr_sub_off(((struct list_node *)__ptr_add_off(i, off))->next, \
> +                         off))

This becomes (untested);

#define list_for_each_off(h, i, off)                                    \
        for (i = list_node_to_off_(list_debug(h)->n.next, (off));       \
             list_node_from_off_(i, (off)) != &(h)->n;                  \
             i = list_node_to_off_(list_node_from_off_(i, (off))->next, off))

> @@ -310,10 +350,57 @@ static inline void list_del_from(struct
> list_head *h, st> ruct list_node *n)
>   *	list_for_each(&parent->children, child, list)
>   *		printf("Name: %s\n", child->name);
>   */
> -#define list_for_each(h, i, member)					\
> -	for (i = container_of_var(list_debug(h)->n.next, i, member);	\
> -	     &i->member != &(h)->n;					\
> -	     i = container_of_var(i->member.next, i, member))
> +#if HAVE_TYPEOF
> +#define list_for_each(h, i, member)                     \
> +  list_for_each_off(h, i, offsetof(typeof(*i), member))
> +#else
> +#define list_for_each(h, i, member)                             \
> +  list_for_each_off(h, i, (char *)&(i)->member - (char *)(i))
> +#endif

Implement of container_of_var_off() as a separate patch?

> +/**
> + * list_for_each_off_safe - iterate through a list of memory regions, maybe
> + * during deletion

Prefer list_for_each_safe_off(), but that's a bit arbitrary.

> diff --git a/ccan/list/test/run.c b/ccan/list/test/run.c
> index f80e364..55c7e2a 100644
> --- a/ccan/list/test/run.c
> +++ b/ccan/list/test/run.c
> @@ -13,6 +13,12 @@ struct child {
>  	struct list_node list;
>  };
>  
> +typedef struct {
> +	struct list_node list;
> +        const char  *name;
> +        unsigned int answer;
> +} opaque_t;
> +

This isn't really opaque though, is it?

There are two ways to make it really opaque.  One is to never define it
at all: malloc memory in char * and treat it as your structure.  The
other is to define a helper.c which has the actual definition (all
unrecognized C files get linked into the tests).

The latter gives you a more realistic test.

Thanks,
Rusty.