svn commit: r192908 - in head: share/man/man9 sys/confsys/kern sys/sys

[Zachary Loafman]

On Wed, May 27, 2009 at 11:28:07AM -0700, Ed Schouten wrote:
> Hi Zach,
> 
> * Zachary Loafman <zml at FreeBSD.org> wrote:
> >   head/sys/sys/queue.h
> 
> This part of the patch is not in the email, so I'll just link it here:
> 
> 	http://svn.freebsd.org/viewvc/base/head/sys/sys/queue.h?r1=191535&r2=192908
> 
> In other macros in this header file, we try to use as many _NEXT() and
> _PREV() invocations as possible, to abstract the structure field names.
> Maybe we could change _SWAP() macros to do the same?

Ed -

I started to do this, but let's run through it:

> #define STAILQ_SWAP(head1, head2, type) do {                            \
>         struct type *swap_first = STAILQ_FIRST(head1);                  \
>         struct type **swap_last = (head1)->stqh_last;                   \
>         STAILQ_FIRST(head1) = STAILQ_FIRST(head2);                      \
>         (head1)->stqh_last = (head2)->stqh_last;                        \
>         STAILQ_FIRST(head2) = swap_first;                               \
>         (head2)->stqh_last = swap_last;                                 \
>         if (STAILQ_EMPTY(head1))                                        \
>                 (head1)->stqh_last = &STAILQ_FIRST(head1);              \
>         if (STAILQ_EMPTY(head2))                                        \
>                 (head2)->stqh_last = &STAILQ_FIRST(head2);              \
> } while (0)

There is no macro that references just ->stqh_last. As it's internal to
the inner workings, I'm not sure there needs to be. The others are
macro-ized already.

> #define LIST_SWAP(head1, head2, type, field) do {                       \
>         struct type *swap_tmp = LIST_FIRST((head1));                    \
>         LIST_FIRST((head1)) = LIST_FIRST((head2));                      \
>         LIST_FIRST((head2)) = swap_tmp;                                 \
>         if ((swap_tmp = LIST_FIRST((head1))) != NULL)                   \
>                 swap_tmp->field.le_prev = &LIST_FIRST((head1));         \
>         if ((swap_tmp = LIST_FIRST((head2))) != NULL)                   \
>                 swap_tmp->field.le_prev = &LIST_FIRST((head2));         \
> } while (0)

Again, there's no macro for .le_prev. Everything else is macro-ized.

> #define TAILQ_SWAP(head1, head2, type, field) do {                      \
>         struct type *swap_first = (head1)->tqh_first;                   \
>         struct type **swap_last = (head1)->tqh_last;                    \
>         (head1)->tqh_first = (head2)->tqh_first;                        \
>         (head1)->tqh_last = (head2)->tqh_last;                          \
>         (head2)->tqh_first = swap_first;                                \
>         (head2)->tqh_last = swap_last;                                  \
>         if ((swap_first = (head1)->tqh_first) != NULL)                  \
>                 swap_first->field.tqe_prev = &(head1)->tqh_first;       \
>         else                                                            \
>                 (head1)->tqh_last = &(head1)->tqh_first;                \
>         if ((swap_first = (head2)->tqh_first) != NULL)                  \
>                 swap_first->field.tqe_prev = &(head2)->tqh_first;       \
>         else                                                            \
>                 (head2)->tqh_last = &(head2)->tqh_first;                \
> } while (0)

This is the only one where we could throw some macro replacements in. But consider this:

>         (head1)->tqh_first = (head2)->tqh_first;                        \
>         (head1)->tqh_last = (head2)->tqh_last;                          \

versus

>         TAILQ_FIRST(head1) = TAILQ_FIRST(head2);                        \
>         (head1)->tqh_last = (head2)->tqh_last;                          \

I personally find the first form clearer to skim through visually. The
second form moves 'head1' and 'head2' around. It's not like it's hard to
follow, but I think the first is clearer to read. I don't think the
TAILQ_FIRST buys you much here.

-- 
Zach Loafman | Staff Engineer | Isilon Systems