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

Translation bounds #148

Merged
merged 1 commit into from
Sep 1, 2017
Merged

Translation bounds #148

merged 1 commit into from
Sep 1, 2017

Conversation

Michael-F-Bryan
Copy link
Contributor

As we discussed in #147 I've removed the unnecessary FromPrimitive bound on the Translate trait.

I also made MapCoords::map_coords() generic over any Fn(&(T, T)) -> (NT, NT) so it uses static dispatch instead of dynamic dispatch and you don't need to write funky &|&(x, y)| { ... } borrowed closures any more.

(fixes #147)

@Michael-F-Bryan
Copy link
Contributor Author

I wasn't able to remove all &'s. Apparently the iterator.map() function can't capture a closure, so on a couple impls I had to pass in a pointer to the closure instead. I'm assuming the compiler/LLVM will be able to dereference these and inline them anyway, so hopefully it turns into a no-op for optimised builds.

@Michael-F-Bryan
Copy link
Contributor Author

Looks like it's not possible to remove the dynamic dispatch in MapCoords::map_coords() because of the way implementations iterate over their components and recursively call map_coords(). This means during the monomorphising stage you end up with an infinitely nested type because the closure is defined in terms of itself.

error[E0275]: overflow evaluating the requirement `[closure@src/algorithm/map_coords.rs:245:34: 245:59]: std::ops::Fn<(&(f64, f64),)>`
  |
  = help: consider adding a `#![recursion_limit="128"]` attribute to your crate
  = note: required because of the requirements on the impl of `std::ops::Fn<(&(f64, f64),)>` for `&[closure@src/algorithm/map_coords.rs:245:34: 245:59]`
  = note: required because of the requirements on the impl of `std::ops::Fn<(&(f64, f64),)>` for `&&[closure@src/algorithm/map_coords.rs:245:34: 245:59]`
...
  = note: required because of the requirements on the impl of `std::ops::Fn<(&(f64, f64),)>` for `&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&[closure@src/algorithm/map_coords.rs:245:34: 245:59]`

I'm going to roll back to 9b4a16b so you can merge that. I'll see if I can figure out a clean way of implementing a MapCoords::map_coords() with static dispatch on a different branch.

@amandasaurus
Copy link
Member

amandasaurus commented Sep 1, 2017 via email

@frewsxcv
Copy link
Member

frewsxcv commented Sep 1, 2017

bors r+

bors bot added a commit that referenced this pull request Sep 1, 2017
@bors
Merge #148
04a9c77 
148: Translation bounds r=frewsxcv a=Michael-F-Bryan

As we discussed in #147 I've removed the unnecessary `FromPrimitive` bound on the `Translate` trait. 

I also made `MapCoords::map_coords()` generic over any `Fn(&(T, T)) -> (NT, NT)` so it uses static dispatch instead of dynamic dispatch and you don't need to write funky `&|&(x, y)| { ... }` borrowed closures any more. 

(fixes #147)
@bors
Copy link
Contributor

bors bot commented Sep 1, 2017

Build succeeded

@bors bors bot merged commit 9b4a16b into georust:master Sep 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Seemingly unnecessary bounds on Translate
3 participants
@Michael-F-Bryan @amandasaurus @frewsxcv