David Rowley <david.row...@2ndquadrant.com> writes: > I've now read over the entire patch and have noted down the following:
Thanks for the review! Attached is a v8 responding to most of your
comments. Anything not quoted below I just accepted.
> 1. MergeAttributes() could do with a comment mention why you're not
> using foreach() on the outer loop.
Check. I also got rid of the Assert added by f7e954ad1, as it seems
not to add any clarity in view of this comment.
> 2. In expandTupleDesc(), couldn't you just change the following:
Done, although this seems like the sort of follow-on optimization
that I wanted to leave for later.
> 3. Worth Assert(list != NIL); in new_head_cell() and new_tail_cell() ?
I don't think so. They're internal functions, and anyway they'll
crash very handily on a NIL pointer.
> 5. Why does enlarge_list() return List *? Can't it just return void?
Also done. I had had some idea of maintaining flexibility, but
considering that we still need the property that a List's header never
moves (as long as it stays nonempty), there's no circumstance where
enlarge_list could validly move the header.
> 9. I see your XXX comment in list_qsort(), but wouldn't it be better
> to just invent list_qsort_internal() as a static function and just
> have it qsort the list in-place, then make list_qsort just return
> list_qsort_internal(list_copy(list)); and keep the XXX comment so that
> the fixup would just remove the list_copy()?
I don't really see the point of doing more than the minimum possible
work on list_qsort in this patch. The big churn from changing it
is going to be in adjusting the callers' comparator functions for one
less level of indirection, and I'd just as soon rewrite list_qsort
in that patch not this one.
> 10. I wonder if we can reduce a bit of pain for extension authors by
> back patching a macro that wraps up a lnext() call adding a dummy
> argument for the List.
I was wondering about a variant of that yesterday; specifically,
I thought of naming the new 2-argument function list_next() not lnext().
Then we could add "#define list_next(l,c) lnext(c)" in the back branches.
This would simplify back-patching code that used the new definition, and
it might save some effort for extension authors who are trying to maintain
cross-branch code. On the other hand, it's more keystrokes forevermore,
and I'm not entirely convinced that code that's using lnext() isn't likely
to need other adjustments anyway. So I didn't pull the trigger on that,
but if people like the idea I'd be okay with doing it like that.
> I also agree with keeping the further improvements like getting rid of
> the needless list_copy() in list_concat() calls as a followup patch. I
> also agree with Tom about moving quickly with this one. Reviewing it
> in detail took me a long time, I'd really rather not do it again after
> leaving it to rot for a while.
Indeed. I don't want to expend a lot of effort keeping it in sync
with master over a long period, either. Opinions?
regards, tom lane
reimplement-List-as-array-8.patch.gz
Description: reimplement-List-as-array-8.patch.gz
