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

Implemented DoubleEndedIterator for EnumIter. #60

Merged
merged 7 commits into from Dec 19, 2019

Conversation

@Nevsor
Copy link
Contributor

@Nevsor Nevsor commented Jul 16, 2019

This is a first working implementation of #59.
All the tests pass including a new one to test the DoubleEndedIterator.

I have never used Rust macros before, so please tell me if something could be implemented better or something is missing.

Maybe it would be more elegant (and performant?) to generate a static or const array instead of the match arms and just return the arrays iterator in iter.

Copy link
Owner

@Peternator7 Peternator7 left a comment

Thanks for the PR! It looks pretty good as an initial implementation. :) The big thing is that iterating forward and backward shouldn't overlap. If you pull some from the back, then they shouldn't show up again when you pull from the front.

strum_macros/src/enum_iter.rs Outdated Show resolved Hide resolved
strum_macros/src/enum_iter.rs Outdated Show resolved Hide resolved
strum_macros/src/enum_iter.rs Outdated Show resolved Hide resolved
strum_tests/tests/enum_iter.rs Show resolved Hide resolved
@Peternator7
Copy link
Owner

@Peternator7 Peternator7 commented Jul 17, 2019

Simply using an iterator to an array is an interesting option. It would require an additional Clone bound on the enum though.

Iterators to arrays return references to the items in the array. EnumIter returns values which means we would need to clone the values returned by the array's iterator to pass them back to the user, and that requires the type be Clone.

You can definitely look at that code to see how they avoid overlapping forward and backward iteration though!

@Nevsor
Copy link
Contributor Author

@Nevsor Nevsor commented Jul 17, 2019

Thank you for the feedback! I did not notice the intended overlapping behaviour on my first reading of the DoubleEndedIterator trait. Unfortunately I will be offline for the rest of the week and will not be able to fix this before tuesday.

@Peternator7
Copy link
Owner

@Peternator7 Peternator7 commented Dec 12, 2019

@Nevsor, are you still working on this?

@Nevsor
Copy link
Contributor Author

@Nevsor Nevsor commented Dec 13, 2019

@Peternator7, sorry for the delay. Here is my implementation of DoubleEndedIterator. I ended up implementing next() and next_back() as special cases of a new, more efficient implementation of nth() and nth_back().

dbc7a25 is not strictly related to this Pull Request, should it be merged seperately?

@Nevsor Nevsor requested a review from Peternator7 Dec 13, 2019
@Peternator7
Copy link
Owner

@Peternator7 Peternator7 commented Dec 19, 2019

Looks great!! Thanks for finishing this up.

@Peternator7 Peternator7 merged commit 8db6a31 into Peternator7:master Dec 19, 2019
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants