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

Enumerable#each_cons support for accepting deque as a reuse buffer #7233

Merged

Conversation

yxhuvud
Copy link
Contributor

@yxhuvud yxhuvud commented Dec 29, 2018

This removes the restriction on Enumerable#each_cons and Iterator#cons to only accept arrays as a container to reuse. With this, it will be possible to use a Deque instead, for better performance.

Also adds some missing tests in iterator_spec.cr

Solves #7189

@yxhuvud yxhuvud changed the title Feat/each cons support for deque Enumerable#each_cons support for accepting deque as a reuse buffer Dec 29, 2018
Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

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

Great!

src/iterator.cr Outdated Show resolved Hide resolved
src/enumerable.cr Outdated Show resolved Hide resolved
src/iterator.cr Outdated Show resolved Hide resolved
@straight-shoota
Copy link
Member

Should reuse == true use a Deque as default for better performance than array?
Or does it need the standard collection type?

@RX14
Copy link
Contributor

RX14 commented Dec 29, 2018

@straight-shoota it'd break a lot of code, since a lot of code only takes arrays.

@yxhuvud
Copy link
Contributor Author

yxhuvud commented Jan 5, 2019

Should I rebase it down to three commits again?

@RX14
Copy link
Contributor

RX14 commented Jan 5, 2019

yeah

spec/std/enumerable_spec.cr Outdated Show resolved Hide resolved
spec/std/enumerable_spec.cr Outdated Show resolved Hide resolved
spec/std/iterator_spec.cr Outdated Show resolved Hide resolved
src/iterator.cr Outdated Show resolved Hide resolved
@yxhuvud yxhuvud force-pushed the feat/each_cons_support_for_deque branch 3 times, most recently from d098cff to 9088392 Compare January 6, 2019 12:19
src/enumerable.cr Outdated Show resolved Hide resolved
@yxhuvud yxhuvud force-pushed the feat/each_cons_support_for_deque branch from 9088392 to 901275f Compare January 6, 2019 18:50
src/enumerable.cr Show resolved Hide resolved
src/iterator.cr Show resolved Hide resolved
src/iterator.cr Show resolved Hide resolved
@yxhuvud yxhuvud force-pushed the feat/each_cons_support_for_deque branch 2 times, most recently from 938a961 to 987257f Compare January 10, 2019 11:47
@yxhuvud yxhuvud force-pushed the feat/each_cons_support_for_deque branch from 987257f to 1c23c29 Compare February 11, 2019 21:00
@yxhuvud
Copy link
Contributor Author

yxhuvud commented Feb 11, 2019

(rebased)

@straight-shoota straight-shoota added this to the 0.28.0 milestone Feb 11, 2019
Copy link
Member

@sdogruyol sdogruyol left a comment

Choose a reason for hiding this comment

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

Thank you @yxhuvud 👍

@sdogruyol sdogruyol merged commit ffcab3b into crystal-lang:master Feb 12, 2019
@yxhuvud yxhuvud deleted the feat/each_cons_support_for_deque branch February 11, 2021 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants