Re: embedded list v2

On Thursday, June 28, 2012 06:23:05 PM Robert Haas wrote:
> On Tue, Jun 26, 2012 at 7:26 PM, Andres Freund <andres@2ndquadrant.com> 
wrote:
> > Attached are three patches:
> > 1. embedded list implementation
> > 2. make the list implementation usable without USE_INLINE
> > 3. convert all callers to ilist.h away from dllist.h
> 
> This code doesn't follow PostgreSQL coding style guidelines; in a
> function definition, the name must start on its own line.
Hrmpf. Yes.

> Also, stuff like this is grossly unlike what's done elsewhere; use the same
> formatting as e.g. foreach:
> +#define ilist_d_reverse_foreach(name, ptr) for(name =
> (ptr)->head.prev;        \
> +                                               name != &(ptr)->head;    \
> +                                               name = name->prev)
Its not only unlike the rest its also ugly... Fixed.

> And this is definitely NOT going to survive pgindent:
> 
> +    for(cur = head->head.prev;
> +        cur != &head->head;
> +        cur = cur->prev){
> +           if(!(cur) ||
> +              !(cur->next) ||
> +              !(cur->prev) ||
> +              !(cur->prev->next == cur) ||
> +              !(cur->next->prev == cur))
> +           {
> +                   elog(ERROR, "double linked list is corrupted");
> +           }
> +    }
I changed the for() contents to one line. I don't think I can write anything 
that won't be changed by pgindent for the if()s contents.

> And this is another meme we don't use elsewhere; add an explicit NULL test:
> 
> +       while ((cur = last->next))
Fixed.

> And then there's stuff like this:
> 
> +           if(!cur){
> +                   elog(ERROR, "single linked list is corrupted");
> +           }
Fixed the places that I found.

> Aside from the formatting issues, I think it would be sensible to
> preserve the terminology of talking about the "head" and "tail" of a
> list that we use elsewhere, instead of calling them the "front" and
> "back" as you've done here.  I would suggest that instead of add_after
> and add_before you use the names insert_after and insert_before, which
> I think is more clear; also instead of remove, I think you should say
> delete, for consistency with pg_list.h.
Heh. Ive been poisoned from using c++ too much (thats partially stl names). 

Changed.

> A number of these inlined functions could be rewritten as macros -
> e.g. ilist_d_has_next, ilist_d_has_prev, ilist_d_is_empty.  Since some
> things are written as macros anyway maybe it's good to do all the
> one-liners that way, although I guess it doesn't matter that much.
I find inline functions preferrable because of multiple evaluation stuff. The 
macros are macros because of the dynamic return type and other similar issues 
which cannot be done in plain C.

> I also agree with your XXX comment that ilist_s_remove should probably
> be out of line.
Done.

Looks good now?

Andres
-- 
 Andres Freund	                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services