Skip to content

Conversation

@jwodder
Copy link
Contributor

@jwodder jwodder commented Oct 28, 2023

  • The fields within EnumIter types (usize and PhantomData) all implement Clone, Copy, Eq, and PartialEq, so we can derive all of those on the iterator types.

  • Once an EnumIter reaches the end of its values, it only ever yields None. This means we can implement FusedIterator on it.

@Peternator7
Copy link
Owner

Hey @jwodder, thanks for the PR! Is this blocking something specific that you're doing? I'm happy to add Clone, and FusedIterator; I'm skeptical of the rest however. Adding Copy will almost definitely cause someone to accidentally mutate a copy rather than the original, and semantically, PartialEq on an iterator seems like an unusual thing to do.

@jwodder
Copy link
Contributor Author

jwodder commented Oct 28, 2023

@Peternator7

Is this blocking something specific that you're doing?

No, it just seemed appropriate.

Adding Copy will almost definitely cause someone to accidentally mutate a copy rather than the original

Fair.

and semantically, PartialEq on an iterator seems like an unusual thing to do.

It doesn't seem unusual to me (though I happen to always derive Eq and PartialEq when possible). On the other hand, I looked over some iterators in std, and the only ones I see that implement Eq are the range types from std::ops, which have non-iterator uses.

Feel free to edit the PR to remove the traits you don't want.

@Peternator7 Peternator7 merged commit 9660ec4 into Peternator7:master Oct 29, 2023
@Peternator7 Peternator7 changed the title Impl Copy, Eq, and FusedIterator for EnumIter iterators Impl FusedIterator for EnumIter iterators Oct 29, 2023
@Peternator7
Copy link
Owner

Thanks for clarifying. After thinking about it, I've merged the FusedIterator addition, but decided to leave the other changes out. Thanks for the update!

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