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 |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
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 | Content-Type | Size |
---|---|---|
0001-Add-embedded-list-interface.patch | text/x-patch | 16.1 KB |
0002-Remove-usage-of-lib-dllist.h-and-replace-it-by-the-n.patch | text/x-patch | 23.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Kevin Grittner | 2012-06-28 18:25:37 | Re: Uh, I change my mind about commit_delay + commit_siblings (sort of) |
Previous Message | Peter Geoghegan | 2012-06-28 18:18:57 | Re: Uh, I change my mind about commit_delay + commit_siblings (sort of) |