Skip site navigation (1) Skip section navigation (2)

Re: embedded list v2

From: Andres Freund <andres(at)2ndquadrant(dot)com>
To: pgsql-hackers(at)postgresql(dot)org
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Tom Lane <tgl(at)postgresql(dot)org>, Peter Geoghegan <peter(at)2ndquadrant(dot)com>
Subject: Re: embedded list v2
Date: 2012-06-28 18:20:59
Message-ID: 201206282020.59376.andres@2ndquadrant.com (view raw or flat)
Thread:
Lists: pgsql-hackers
On Thursday, June 28, 2012 06:23:05 PM Robert Haas wrote:
> On Tue, Jun 26, 2012 at 7:26 PM, Andres Freund <andres(at)2ndquadrant(dot)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

Attachment: 0002-Remove-usage-of-lib-dllist.h-and-replace-it-by-the-n.patch
Description: text/x-patch (23.6 KB)
Attachment: 0001-Add-embedded-list-interface.patch
Description: text/x-patch (16.1 KB)

In response to

Responses

pgsql-hackers by date

Next:From: Kevin GrittnerDate: 2012-06-28 18:25:37
Subject: Re: Uh, I change my mind about commit_delay + commit_siblings (sort of)
Previous:From: Peter GeogheganDate: 2012-06-28 18:18:57
Subject: Re: Uh, I change my mind about commit_delay + commit_siblings (sort of)

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group