[zeromq-dev] Proposal for a revised zlist API

[Pieter Hintjens]

On Fri, Sep 5, 2014 at 3:56 PM, Goswin von Brederlow <goswin-v-b at web.de> wrote:

> Just saw your commit.

You only found 17 issues? :-) I was coding till 1am, surprising it
worked at all.

> 1) Small typos

Fixed.

> 2) zlist_purge does not destroy the cursor allowing zlist_next and
> zlist_item access to freed memory.

Nice catch, fixed.

> 3) change cursor to "node_t **" but that would be internal.

I'm not yet sure of that change, will try something else first
(separate head/tail item as we have in zring, I think).

> 4) Now there is zlist_detach and zlist_delete. How about
> zlist_detach_current and zlist_delete_current for the cursor based
> flavour of the same? (needs 3)

Not needed, you pass NULL as item and that does the job. I don't like
long method names either... clumsy to use.

> 5) zlist_set_comparator for a container wide compare function.

Tried that and decided against it, due to not wanting to break
zlist_sort and not really seeing the benefit (in fact it's an extra
line of code and extra risk of errors).

> 6) zlist_sort could allow NULL as second argument to use the container
> compare (or compare by address, however useless that is :). (needs 5)

Yes, that was my second idea, except it leaves the method messy and I
still didn't see the benefit.

> 7) zlist_find can be added without name collision. (needs 5)

So searching on content rather than item value, yes, it could work.

> 8) zlist_detach and zlist_delete need to specify their affect on the
> cursor.

zlist_detach and zlist_delete are works in progress, I left this for
today as I had other work. They should end up working and looking like
their zring counterparts.

> 9) I would suggest having zlist_detach and zlist_delete use container
> compare function.

OK, I'm partially convinced, however I don't have a use case, this is
engineering because "it can work" which I'd rather avoid. So far
comparison on the item void * has always worked fine.

> 10) I would suggest having zlist_detach and zlist_delete use zlist_find
> + zlist_*_current. (needs 7, solves 9 implicitly)

Again, problem statements first, solutions second.

> 11) zlist_t still has a "bool autofree;". The old behaviour could be
> achieved by having zlist_autofree set a duplicator (strdup) and
> destructor (something that calls free) instead.

Indeed. I'd randomly thought that and didn't do it yet. My goal today
was the general shape of the API and keeping existing code from not
breaking.

> 12) Can we add the "void *user" argument to czmq_destructor and
> czmq_comparator? That would be realy usefull for keeping items in 2
> container like zcertstore does among other things.

Haven't understood the problem yet enough to evaluate the addition.
I'll get there eventually, if you can wait.

> 13) zlist_first, zlist_next and zlist_last each duplicate zlist_item
> at the end.

Same in zring. I'll fix that. zlist_head too.

> 14) zlist_insert_before could be added without name collision. (needs 3)

I've added and removed that twice now, and still don't see a use case for it.

> 15) zlist_insert(_after) could be added without name collision.
> 16) zlist_insert_sorted could be added without name collision. (needs 3+5+7)

OK, enough! :-) It is painful to wrangle APIs, and it gets worse
whenever we add stuff we do not absolutely need. Please come with
problems that we cannot reasonably solve using the current APIs, or
places where we can make valuable improvements.

> 17) zlist_last is a problem for 4 since it updates the cursor.
> Consider this stupid way to purge the list in reverse order:
>
>     while (zlist_last (list)) zlist_delete_current (list);

zlist_purge (). I don't really care about stupid use cases.

However, zlist_last uses the list tail. Any delete/detach has to
update the tail.

> If someone needs to process a zlist from the back like that then they
> should be using a zring instead. So I think this would be safe.

Indeed. There are some basic styles we use lists for, and that's all I
really care about. Weird constructs don't count as valid use cases
since they're usually wrong in other subtle ways anyhow.

> 18) You've changed the API, the cursor behaviour, slightly already. I
> wonder if any code relies on the destructive cursor behaviour of
> zlist_push, zlist_append, zlist_remove (and any others I forgot). It
> is possible to have code like this:

I'm going to deprecate that notion that zlist_next() makes sense
without a zlist_first(). It's errorprone and silly to have magical
assumptions about list state that you can't visually verify, and can't
copy/paste as a block.

-Pieter