From: | Neil Conway <neilc(at)samurai(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | PostgreSQL Patches <pgsql-patches(at)postgresql(dot)org> |
Subject: | Re: next draft of list rewrite |
Date: | 2004-01-25 23:27:32 |
Message-ID: | 871xpna9sr.fsf@mailbox.samurai.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-patches |
Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> I thought the plan was to keep the API exactly the same as before,
> with the single exception of having to use ListCell* rather than
> List* as the type of pointers used to iterate through a list.
I thought that once we had decided to change the API, anything was
fair game: either we need to change every call site of the API (which
would be required if we adopted ListCell* as the type of the foreach
iteration variable), or we don't.
> It will undoubtedly break a fair amount of user-written code that we
> don't have control over
Granted, but so will changing the API at all: as above, the ListCell*
change would cause approximately the same amount of breakage for
users.
> Everyone who works on the backend is already accustomed to the
> existing names, which are mostly derived from ancient Lisp
> tradition.
The heritage of the names was part of my reason for changing them: the
Lisp names imply a certain style of list implementation (cons cells,
linear-time length and append ops but constant time prepend) that is
no longer being used. In addition, that makes names like lfirst()
(which would now be applied to a ListCell*) no longer very meaningful.
In addition, there are plenty of infelicities that it would be nice to
remove from the API. For example:
- names like lispRemove() that aren't very meaningful (AFAICS),
even in the present implementation
- some names use case to separate words (e.g. LispRemove(),
freeList()), some use underscores (e.g. set_union()), while
some use nothing (e.g. llastnode())
- some names begin with capital letters (e.g. FastAppend()),
the rest do not
- some names use a 'ptr/int/oid' prefix to denote type-specific
variants of a function, whereas some use a 'i' or 'o' suffix
for the same purpose.
- some names are prefixed with 'l', some are not -- without
apparently rhyme or reason for this distinction
- some functions take the List* as their first argument
(e.g. lappend()), some do not (e.g. nth()) -- again, without
any good reason for the inconsistency that I could see
> If you want to push forward on it, we had better have a vote in
> pghackers, rather than assuming everyone concerned will see this
> discussion in -patches.
I'm still open to being convinced, BTW: I only write the code as it is
without prior discussion because I misunderstood your earlier
position. I'll send a mail to hackers if we're still in disagreement
at the end of this thread.
> I noticed one trivial bug, which is that the Asserts in
> list_member_int and list_member_oid seem backwards.
Thanks: Alvaro was already kind enough to point that out via email.
> A bigger problem is that list_length, list_head, and list_tail must
> *not* be macros --- we cannot afford double-evaluation bugs
> associated with operations as widely used as these.
Fair enough. I'll also take a look at GCC-style inline functions...
> No, you'd simply leave that cell-space wasted if the head member
> were to change. It's doable, though it would complicate the
> deletion routine and probably nconc as well. It's entirely likely
> that it'd be more complexity than it's worth, though, so if you
> don't want to do it that's fine with me. Certainly it makes sense
> not to do it in the first pass.
Ok, I won't worry about it for now.
> [ a scheme for iteratively committing the changes ]
> This is a lot more pleasant way to proceed than a "big bang"
> changeover.
I agree that your method is easier procedurally, but ISTM that the
aggregate amount of work required is about the same: we still need to
change pretty much every call site of the list API. Yes, it is
slightly easier if we can do this one call site at a time, and yes,
it's slightly easier if s/List/ListCell/ is the only required change,
but there is about the same degree of work involved either
way -- and IMHO the benefit of a well named API is worth the
short-term pain.
Furthermore, it is quite possible to reduce the pain induced by my
method. For example, we could create a private CVS branch. In that
branch, it wouldn't matter if the tree is broken for a week or two, in
which time we can make the changes across the tree at our leisure, and
then merge them into HEAD in one relatively painless branch landing.
-Neil
From | Date | Subject | |
---|---|---|---|
Next Message | Neil Conway | 2004-01-25 23:36:08 | Re: appendStringInfoString() micro-opt |
Previous Message | Tom Lane | 2004-01-25 23:07:36 | Re: ANALYZE patch for review |