disque

disque copied to clipboard

Hi @antirez , I saw that skiplist lacks Iter APIs like dict, adlist etc. Hope this PR could be helpful. :-)

Hello @sunheehnus, this is in general a good idea, but the skiplist is so easy to navigate and requires so little state that I would make it more like a stack allocated structure and a few macros or alike. The adlist iterator is already a bit like that if you use listRewind(), you just declare an iterator var and iterate, we could do the same?

skiplistIterator si;
skiplistRewind(myskiplist,&si);
element = skiplistNext(&si);

Bonus point if we implement even skiplistNext as macro if possible, otherwise we could make it a small inline function inside skiplist.h. Makes sense? So we have a similar API to what you propose but without any dynamic allocation or overhead.

So:

1. skiplistRewind / skiplistRewindTail would be both macros, and that's easy.
2. skiplistNext would be, possibly a macro, or if it gets dirty, an line.
3. No cleanup of the iterator needed :+1:

Makes sense?

Hi @antirez , Thanks very much for you feedback. :-) Your advice do make sense. And I already follow your advice with the new patch.

• remove dynamic allocation APIs.
• skiplistRewind/skiplistRewindTail changed to macros
• skiplistNext changed to an inline func

It could be more efficient without allocating memory dynamically. In the original version, I just imitate the APIs of adlist. As far as I review, listGetIterator() is not used outside adlist.c. Actually the usage of it inside adlist.c could be substitute with listRewind(). :-)