Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skiplist: add Iter APIs #67

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

sunheehnus
Copy link
Contributor

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

@antirez
Copy link
Owner

antirez commented May 15, 2015

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 👍

Makes sense?

Follow antirez's advice in antirez#67
skiplistRewind/skiplistRewindTail changed to macros,
skiplistNext changed to an inline func,
Iterator should be guaranteed stack allocated and no cleanup need.
@sunheehnus
Copy link
Contributor Author

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().
:-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants