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

Add a chain! macro. #525

Merged
merged 2 commits into from Mar 1, 2021
Merged

Add a chain! macro. #525

merged 2 commits into from Mar 1, 2021

Conversation

cassiersg
Copy link
Contributor

This is proposal to add a chain! macro which creates an iterator running multiple iterators sequentially.

It basically calls multiple times the standard .chain(). This macro is useful to avoid rightwards shift of the code when chaining many iterators.

For the implementation and tests, I took inspiration on the izip! macro.

Copy link
Member

@phimuemue phimuemue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi there! I sometimes thought about such a facility, but I never got around to actually writing it. If we decide to keep it, I'd find it nice to support 0 iterators, too.

src/lib.rs Outdated Show resolved Hide resolved
tests/quick.rs Outdated
Comment on lines 531 to 533
fn size_chain_macro(a: Iter<i16, Exact>, b: Iter<i16, Exact>, c: Iter<i16, Exact>) -> bool {
correct_size_hint(chain!(a.clone(), b.clone(), c.clone()))
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope I don't miss anything, but is size_hint special for chain!? What about other potentially specialized methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not special. I guess this test shouldn't be there (I juste copied some of the izip! tests) since it tests more the properties of the std chain: we don't specialize any method in this crate. I don't see how the macro could be wrong while chain being correct.

If we were to verify all the methods that are specialized by Chain, that would mean writing quite many tests (here is the list).

The chain! macro creates an iterator running multiple iterators
sequentially.
@cassiersg
Copy link
Contributor Author

@phimuemue I believe this is ready for a second review.

@phimuemue
Copy link
Member

Note: I did not test this, but it looks good to me. As far as I can see, one can call chain(iter1, iter2, ,,,) (note trailing commas). For me this would be ok, but let's wait for @jswrenn's opinion.

As I do not have that much time to review this, I'd appreciate a second pair eyes on this, to rule out potential pitfalls.

Copy link
Member

@jswrenn jswrenn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, and sorry for being so slow at reviewing; I've been super tied up lately.

I wonder if it might also make sense to provide a Chain! type macro that expands to the type produced by chain!, but that doesn't have to be in this PR. I think it might be somewhat tricky to implement, too...

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Co-authored-by: Jack Wrenn <me@jswrenn.com>
@cassiersg
Copy link
Contributor Author

@jswrenn Thanks for the suggestions !

@jswrenn jswrenn added this to the next milestone Mar 1, 2021
Copy link
Member

@jswrenn jswrenn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 1, 2021

Build succeeded:

@bors bors bot merged commit 59cb6f5 into rust-itertools:master Mar 1, 2021
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.

None yet

4 participants