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

Implemented minmax_by #138

Merged
merged 1 commit into from
Sep 7, 2016
Merged

Implemented minmax_by #138

merged 1 commit into from
Sep 7, 2016

Conversation

phimuemue
Copy link
Member

I implemented a basic version of minmax_by (similar to sort_by).

Note that it is (at least, currently) implemented to take an Fn (instead of an FnMut) as a comparison function. Do you think a FnMut comparison function would offer sensible advantages?

Just in case this matters: I also suggested to add max_by and min_by to the stdlib, so minmax_by would be in line with that (rust-lang/rust#35856).

@bluss
Copy link
Member

bluss commented Aug 21, 2016

I use FnMut by "default", i.e everywhere it sensibly works. This should be an FnMut, that just permits more closures to be used (for example those modifying an upvar for some reason). Could be any reason, including modifying something for debugging/logging purposes.

The usual reason to restrict to Fn is if you need shared ownership/shared access or other concurrency, is how I understand it.

@bluss
Copy link
Member

bluss commented Aug 21, 2016

It would be even cooler if there was a quickcheck test for minmax.

@bluss
Copy link
Member

bluss commented Sep 7, 2016

I'd like to merge this. Using Fn over FnMut is required right, since we're sharing the comparison function between two locations (without some refactoring or internal mutability)?

@bluss
Copy link
Member

bluss commented Sep 7, 2016

Using this, the iris.rs example can be updated to use minmax_by too.

@phimuemue
Copy link
Member Author

@bluss You're right, we are sharing the comparison function as of now.

Just my two cents: I suggest merging this pull request (after all, having minmax_by with an Fn is probably better than not having it at all), and opening a ticket regarding the Fn/FnMut issue (which I could take as soon as I have time).

@bluss
Copy link
Member

bluss commented Sep 7, 2016

At the same time, path of least resistance usually leads us to simple, non-hacky solutions. It's not worth it to use interior mutability I think.

@bluss bluss merged commit 97028c3 into rust-itertools:master Sep 7, 2016
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