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

Minmax set (prev "Add {min,max}_set(_by{_key)?)? functions") #613

Merged
merged 8 commits into from May 23, 2022

Conversation

phimuemue
Copy link
Member

This PR supersedes #323 (I did not know how I could amend to the original PR, so sorry for the "duplicate" PR here.)

The original PR by @zayenz looked rather good. I adjusted the stuff that came up during discussion back then:

  • Teturn type Vec instead of Option - emptiness is sufficiently well represented by Vec.
  • Functions require Ord instead of PartialOrd - just as Iterator::min.
  • Avoid duplicate calls to lt by accepting a FnMut(...)->Ordering - seems canonical compared to the bool-solution.
  • Use internal iteration instead of a manual for-loop.

Moreover, I simplified some bits.

zayenz and others added 7 commits May 4, 2022
The function min_set returns a Vec of all the minimum values. All
variants for max and with key extraction and comparison functions are
added also.

Since the functions need to return an unknown number of values and the
values are not known until the iterator is finished, the function
needs to allocate memory. Therefore Vec is used for returning the
values.
…alOrd

We may relax this bound at some point, but I'd go with this until we
have evidence that it bothers users, as there have been cases where I
 actually was happy to be informed about Ord vs PartialOrd.
We may relax this bound at some point, but I'd go with this until we
have evidence that it bothers users, as there have been cases where I
 actually was happy to be informed about Ord vs PartialOrd.
Slightly simplifies control flow as it avoids mutable variables that may
not even be used in case of early return.
the {min,max}_set results must contain the result of the corresponding
{min,max} variant.
@phimuemue
Copy link
Member Author

phimuemue commented May 21, 2022

@jswrenn @zayenz If there are no objections, I'd merge this.

@zayenz
Copy link
Contributor

zayenz commented May 22, 2022

This looks great to me and thanks for taking over and fixing it.

@jswrenn
Copy link
Member

jswrenn commented May 23, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented May 23, 2022

Build succeeded:

@bors bors bot merged commit 1cb2c36 into rust-itertools:master May 23, 2022
4 checks passed
@jswrenn jswrenn added this to the next milestone May 23, 2022
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.

None yet

3 participants