Thank you for having another look at this
On Sat, 29 Oct 2022 at 18:32, Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
> 1. I guess we need to cast the 'node' parameter too, something like
> below? I'm reading the comment there talking about compilers
> complaning about the unused function arguments.
> dlist_member_check(head, node) ((void) (head); (void) (node);)
I looked at dlist_check() and I didn't quite manage to figure out why
the cast is needed. As far as I can see, there are no calls where we
only pass dlist_head solely for the dlist_check(). For
dlist_member_check(), dlist_delete_from() does not use the 'head'
parameter for anything apart from dlist_member_check(), so I believe
the cast is required for 'head'. I think I'd rather only add the cast
for 'node' unless we really require it. Cargo-culting it in there just
because that's what the other macros do does not seem like a good idea
to me.
> Can we put max limit, at least in assert, something like below, on
> 'count' instead of saying above? I'm not sure if there's someone
> storing 4 billion items, but it will be a good-to-have safety from the
> data structure perspective if others think it's not an overkill.
> Assert(head->count > 0 && head->count <= PG_UINT32_MAX);
'count' is a uint32. It's always going to be <= PG_UINT32_MAX.
My original thoughts were that it seems unlikely we'd ever give an
assert build a workload that would ever have 2^32 dlist_nodes in
dclist. Having said that, perhaps it would do no harm to add some
overflow checks to 'count'. I've gone and added some
Assert(head->count > 0) after we do count++.
> + * As dlist_delete but performs checks in ILIST_DEBUG to ensure that 'node'
> + * belongs to 'head'.
> I think 'Same as dlist_delete' instead of just 'As dlist_delete'
I don't really see what's wrong with this. We use "As above" when we
mean "Same as above" in many locations. Anyway, I don't feel strongly
about not adding the word, so I've adjusted the wording in that
comment which includes adding the word "Same" at the start.
> 5.
> + * Caution: 'node' must be a member of 'head'.
> + * Caller must ensure that 'before' is a member of 'head'.
> Can we have the same comments around something like below?
I've adjusted dclist_insert_after() and dclist_insert_before(). Each
dclist function that uses dlist_member_check() now has the same text.
> 8. Wondering if we need dlist_delete_from() at all. Can't we just add
> dlist_member_check() dclist_delete_from() and call dlist_delete()
> directly?
Certainly, but I made it that way on purpose. I wanted dclist to have
a superset of the functions that dlist has. I just see no reason why
dlist shouldn't have dlist_delete_from() when dclist has it.
I've attached the v3 version of the patch which includes some
additional polishing work.
David
