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

Change closure for exterior_mut() and interiors_mut() to be FnOnce. #479

Merged
merged 1 commit into from
Jul 20, 2020

Conversation

jsadusk
Copy link
Contributor

@jsadusk jsadusk commented Jul 20, 2020

The current form of exteriors_mut accepts a closure of FnMut. This causes this case not to compile:

fn update_exterior(poly: &mut Polygon<i64>) {
  let new_exterior = get_some_linestring();
  poly.exterior_mut(|exterior| *exterior = new_exterior);
}

While this does:

fn update_exterior(poly: &mut Polygon<i64>) {
  let new_exterior = get_some_linestring();
  poly.exterior_mut(|exterior| *exterior = new_exterior.clone());
}

FnMut is to allow a closure to have a mutable state, so that it can be called multiple times, modifying that state. The closure is only called once, and the part that is mutable is the parameter to it, so it doesn't need to be an FnMut. FnOnce is a superset of FnMut, so changing the trait bound to be FnOnce should have no affect on existing use cases, while enabling new ones.

closure is only called once, so it doesn't need to be an FnMut, and
FnMut prevents the closure from moving a captured variable into
exterior or interiors
@jsadusk jsadusk changed the title Change closure for exterior_mut() and interiors_mut() to be FnOnce. Change closure for exterior_mut() and interiors_mut() to be FnOnce. Jul 20, 2020
Copy link
Member

@frewsxcv frewsxcv left a comment

Choose a reason for hiding this comment

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

good catch, thanks!

@frewsxcv frewsxcv merged commit 63dd60d into georust:master Jul 20, 2020
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