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

Avoid Box in methods returning iterators #218

Merged
merged 1 commit into from
May 12, 2018
Merged

Avoid Box in methods returning iterators #218

merged 1 commit into from
May 12, 2018

Conversation

lnicola
Copy link
Member

@lnicola lnicola commented May 12, 2018

Part of #198.

This only uses impl Iterator for lines. The other methods can't use it because they're part of a trait.

The code is a little ugly, and replacing Points with impl Trait in the future will be a breaking change. Feel free to close if you think it's not worth it.

For what it's worth, if we do switch to impl Iterator in the future, writing down the ugly EitherIter<&'a Point<T>, Iter<'a, Point<T>>, Rev<Iter<'a, Point<T>>>> type will no longer be needed.

@frewsxcv
Copy link
Member

Thanks!

bors r+

bors bot added a commit that referenced this pull request May 12, 2018
218: Avoid Box in methods returning iterators r=frewsxcv a=lnicola

Part of #198.

This only uses `impl Iterator` for `lines`. The other methods can't use it because they're part of a trait.

The code is a little ugly, and replacing `Points` with `impl Trait` in the future will be a breaking change. Feel free to close if you think it's not worth it.

For what it's worth, if we do switch to `impl Iterator` in the future, writing down the ugly `EitherIter<&'a Point<T>, Iter<'a, Point<T>>, Rev<Iter<'a, Point<T>>>>` type will no longer be needed.

Co-authored-by: Laurentiu Nicola <lnicola@dend.ro>
@lnicola
Copy link
Member Author

lnicola commented May 12, 2018

Maybe it would be worth noting the minimum compiler version in the release notes?

@bors
Copy link
Contributor

bors bot commented May 12, 2018

Build succeeded

@bors bors bot merged commit a56c52e into georust:master May 12, 2018
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