Hi,
On 2020-02-21 12:40:06 +1300, Thomas Munro wrote:
> On Thu, Feb 20, 2020 at 5:14 PM Andres Freund <and...@anarazel.de> wrote:
> > 16 files changed, 569 insertions(+), 1053 deletions(-)
>
> Nice!
Thanks!
> Some comments on 0001, 0003, 0004:
>
> > Subject: [PATCH v1 1/6] Add additional prev/next & detached node functions
> > to
>
> +extern void dlist_check(const dlist_head *head);
> +extern void slist_check(const slist_head *head);
>
> I approve of the incidental constification in this patch.
It was just necessary fallout :)
> +/*
> + * Like dlist_delete(), but also sets next/prev to NULL to signal not being
> in
> + * list.
> + */
> +static inline void
> +dlist_delete_thoroughly(dlist_node *node)
> +{
> + node->prev->next = node->next;
> + node->next->prev = node->prev;
> + node->next = NULL;
> + node->prev = NULL;
> +}
>
> Instead of introducing this strange terminology, why not just have the
> callers do ...
>
> dlist_delete(node);
> dlist_node_init(node);
There's quite a few callers in predicate.c - I actually did that first.
> ..., or perhaps supply dlist_delete_and_reinit(node) that does exactly
> that? That is, reuse the code and terminology.
Yea, that's might be better, but see paragraph below. I quite dislike
adding any "empty node" state.
> +/*
> + * Check if node is detached. A node is only detached if it either has been
> + * initialized with dlist_init_node(), or deleted with
> + * dlist_delete_thoroughly().
> + */
> +static inline bool
> +dlist_node_is_detached(const dlist_node *node)
> +{
> + Assert((node->next == NULL && node->prev == NULL) ||
> + (node->next != NULL && node->prev != NULL));
> +
> + return node->next == NULL;
> +}
>
> How about dlist_node_is_linked()? I don't like introducing random new
> verbs when we already have 'linked' in various places, and these
> things are, y'know, linked lists.
Well, but that doesn't signal that you can't just delete and have
dlist_node_is_linked() work. I *want* it to sound "different". We could
of course make delete always do this, but I don't want to add that
overhead unnecessarily.
> > Subject: [PATCH v1 4/6] Use dlists for predicate locking.
>
> + dlist_foreach(iter, &unconstify(SERIALIZABLEXACT *,
> reader)->outConflicts)
>
> Yuck...
It doesn't seem *that* bad to me, at least signals properly what we're
doing, and only does so in one place.
> I suppose you could do this:
>
> - dlist_foreach(iter, &unconstify(SERIALIZABLEXACT *,
> reader)->outConflicts)
> + dlist_foreach_const(iter, &reader->outConflicts)
We'd need a different iterator type too, I think? Because the iterator
itself can't be constant, but we'd want the elements themselves be
pointers to constants.
const just isn't a very granular thing in C :(.
> ... given:
>
> +/* Variant for when you have a pointer to const dlist_head. */
> +#define dlist_foreach_const(iter, lhead) \
> + for (AssertVariableIsOfTypeMacro(iter, dlist_iter), \
> + AssertVariableIsOfTypeMacro(lhead, const dlist_head *), \
> + (iter).end = (dlist_node *) &(lhead)->head, \
> + (iter).cur = (iter).end->next ? (iter).end->next : (iter).end; \
> + (iter).cur != (iter).end; \
> + (iter).cur = (iter).cur->next)
> +
>
> ... or find a way to make dlist_foreach() handle that itself, which
> seems pretty reasonable given its remit to traverse lists without
> modifying them, though perhaps that would require a different iterator
> type...
I was trying that first, but I don't easily see how we can do
so. Iterating over a non-constant list with dlist_foreach obviously
still allows to to manipulate the list members. Thus dlist_iter.cur
can't be a 'pointer to const'. Whereas that's arguably what'd be needed
for a correct dlist_foreach() of a constant list?
We could just accept const pointers for dlist_foreach(), but then we'd
*always* accept them, and we'd thus unconditionally have iter.cur as
non-const. Would that be better?
> Otherwise looks OK to me and passes various tests I threw at it
Thanks!
Greetings,
Andres Freund
