Re: embedded list v2
От | Andres Freund |
---|---|
Тема | Re: embedded list v2 |
Дата | |
Msg-id | 201206282020.59376.andres@2ndquadrant.com обсуждение исходный текст |
Ответ на | Re: embedded list v2 (Robert Haas <robertmhaas@gmail.com>) |
Ответы |
Re: embedded list v2
(Alvaro Herrera <alvherre@commandprompt.com>)
Re: embedded list v2 (Peter Geoghegan <peter@2ndquadrant.com>) Re: embedded list v2 (Alvaro Herrera <alvherre@2ndquadrant.com>) |
Список | 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@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
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Peter GeogheganДата:
Сообщение: Re: Uh, I change my mind about commit_delay + commit_siblings (sort of)
Следующее
От: "Kevin Grittner"Дата:
Сообщение: Re: Uh, I change my mind about commit_delay + commit_siblings (sort of)