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.

Sign up for GitHub

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

Jump to bottom

CoordinatesIter does not implement ExactSizeIterator correctly, panics #762

Closed
seritools opened this issue Mar 10, 2022 · 3 comments · Fixed by #766
Closed

CoordinatesIter does not implement ExactSizeIterator correctly, panics #762

seritools opened this issue Mar 10, 2022 · 3 comments · Fixed by #766

Comments

@seritools
Copy link

This comes from the rust discord server, where someone tried to iterate over all but the last point of a LineString (being the exterior of a Polygon). In the meantime we realized just skipping the first one works just as fine for their usecase, but before that we stumbled upon this panic:

From https://doc.rust-lang.org/std/iter/trait.ExactSizeIterator.html:

When implementing an ExactSizeIterator, you must also implement Iterator. When doing so, the implementation of Iterator::size_hint must return the exact size of the iterator.

This means that something like

for coord in line_string.into_iter().rev().skip(1).rev() { ... }

panics, failing this assertion: https://doc.rust-lang.org/src/core/iter/traits/exact_size.rs.html#103-108

(It would also be nice for PointsIter to implement ExactSizeIterator as well I think :P)

I haven't looked at other iterators in the crate, but they might also have this issue.
@urschrei
Copy link
Member

Hi, could you provide a complete failing test case?

@seritools
Copy link
Author

seritools commented Mar 11, 2022 •
edited
Loading

Sure thing, here you go, this panics:

use geo::{Coordinate, LineString};

fn main() {
    let ls = LineString(vec![
        Coordinate { x: 0., y: 0. },
        Coordinate { x: 10., y: 0. },
    ]);

    // reference to force the `impl IntoIterator for &LineString` impl, giving a `CoordinatesIter`
    for c in (&ls).into_iter().rev().skip(1).rev() {
        println!("{:?}", c);
    }
}

@urschrei
Copy link
Member

Thanks, just wanted to make sure I wasn't misunderstanding.

@urschrei urschrei mentioned this issue Mar 12, 2022
Merged
2 tasks
urschrei added a commit to urschrei/geo that referenced this issue Mar 12, 2022
@urschrei
Add ExactsizeIterator impl for Points iterator on LineString
267876a 
Follow-up to georust#762
@urschrei urschrei mentioned this issue Mar 12, 2022
Merged
2 tasks
bors bot added a commit that referenced this issue Mar 12, 2022
@bors @urschrei
Merge #766
f0989bc 
766: Add missing size_hint method on LineString iterators r=michaelkirk a=urschrei

Fixes #762

- [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md).
- [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.
---



Co-authored-by: Stephan Hügel <shugel@tcd.ie>
@bors bors bot closed this as completed in b83dc75 Mar 12, 2022
@bors bors bot closed this as completed in #766 Mar 12, 2022
urschrei added a commit to urschrei/geo that referenced this issue Mar 13, 2022
@urschrei
Add ExactsizeIterator impl for Points iterator on LineString
0ac1109 
Follow-up to georust#762
bors bot added a commit that referenced this issue Mar 13, 2022
@bors @urschrei
Merge #767
d1c6d38 
767: Add ExactsizeIterator impl for Point iterator on LineString r=urschrei a=urschrei

- [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md).
- [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.
---

Follow-up to #762 and #766



Co-authored-by: Stephan Hügel <shugel@tcd.ie>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add missing size_hint method on LineString iterators urschrei/geo
2 participants
@urschrei @seritools