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 Enumerable#each_cons_pair and Iterator#cons_pair yielding a tuple #8332

Merged

Conversation

@straight-shoota
Copy link
Member

straight-shoota commented Oct 15, 2019

Enumerable#each_cons has a variable chunk size that can be selected dynamically.
This PR adds a special case for the most common use case of iterating over pairs. It simply yields tuples of adjacent items. This avoids heap allocations and makes it easier to use because you don't need to deconstruct an array.

# new:
[1, 2, 3, 4, 5].each_cons do |a, b|
  puts a * b
end
# old:
[1, 2, 3, 4, 5].each_cons(2, reuse: true) do |pair|
  a, b = pair
  puts a * b
end

Benchmark:

# iterating over (1..1000).to_a
new 228.27k (  4.38µs) (±17.91%)   0.0B/op        fastest
old  33.13k ( 30.19µs) (± 9.87%)  80.0B/op   6.89× slower

It's a convenience feature that also improves performance.

I've added the same behaviour to Iterator#cons

@asterite

This comment has been minimized.

Copy link
Member

asterite commented Oct 15, 2019

I like this!

What if we name it each_cons2? Then we could eventually add each_cons3, each_cons4, etc. if we really wanted to.

Though it's true that one of the most common cases for each_cons is 2... 🤔

@straight-shoota

This comment has been minimized.

Copy link
Member Author

straight-shoota commented Oct 15, 2019

I don't see a reason for adding a number suffix. Iterating over pairs is by far the most common use case. If we wanted to add others, there's nothing hindering us from adding each_cons3 later anyway.

@RX14
RX14 approved these changes Oct 29, 2019
@RX14 RX14 added this to the 0.32.0 milestone Oct 29, 2019
@RX14

This comment has been minimized.

Copy link
Member

RX14 commented Oct 29, 2019

Want to rebase for a merge commit?

@straight-shoota straight-shoota force-pushed the straight-shoota:feature/each_cons-2 branch from 5510382 to fd01717 Oct 29, 2019
@vlazar

This comment has been minimized.

Copy link
Contributor

vlazar commented Oct 29, 2019

I love this optimization but I feel like the chosen size 2 mentioned explicitly in the method name like #each_cons_pair / Iterator#cons_pair would be even better.

@bcardiff

This comment has been minimized.

Copy link
Member

bcardiff commented Oct 29, 2019

I like @vlazar's suggestion. Otherwise, the expected block arguments differ based on the presence or not of the argument. I like the explicitness of each_cons_pair

@straight-shoota

This comment has been minimized.

Copy link
Member Author

straight-shoota commented Oct 29, 2019

I guess that might be a good enhancement, but waiting for feedback on this suggestion.

@asterite

This comment has been minimized.

Copy link
Member

asterite commented Oct 29, 2019

I think each_cons_pair is good. My suggestion each_cons2, each_cons3, etc. might also be taken into consideration, though I doubt we'll spend effort adding each of these other special variants, so 👍 each_cons_pair from me.

@RX14

This comment has been minimized.

Copy link
Member

RX14 commented Nov 1, 2019

+1 from me, might want to go ahead with this

@bcardiff

This comment has been minimized.

Copy link
Member

bcardiff commented Nov 1, 2019

each_cons_pair or each_cons2 should be used as a name. Other than that LGTM.

@straight-shoota straight-shoota force-pushed the straight-shoota:feature/each_cons-2 branch from fd01717 to 061e4a4 Nov 1, 2019
@straight-shoota

This comment has been minimized.

Copy link
Member Author

straight-shoota commented Nov 1, 2019

Renamed to Enumerable#each_cons_pair and Iterator#cons_pair.

Copy link
Member

bcardiff left a comment

Merge upon renaming the PR for the changelog.

@straight-shoota straight-shoota changed the title Let #each_cons yield a tuple Add Enumerable#each_cons_pair and Iterator#cons_pair yielding a tuple Nov 1, 2019
@straight-shoota straight-shoota merged commit 6d17522 into crystal-lang:master Nov 1, 2019
6 checks passed
6 checks passed
ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32_std Your tests passed on CircleCI!
Details
ci/circleci: test_preview_mt Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@straight-shoota straight-shoota deleted the straight-shoota:feature/each_cons-2 branch Nov 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.