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

Rename first*/last* BTree{Set,Map} methods to min*/max* #93709

Closed

Conversation

WaffleLapkin
Copy link
Member

@WaffleLapkin WaffleLapkin commented Feb 6, 2022

On the tracking issue there a lot of discussions on the names of these methods.

I propose that

  1. libs team should make a final decision about the names, so we can move towards stabilization
  2. the names should be changed to min*/max* (reasoning below)

TL;DR this PR renames first*/last* methods of BTreeMap and BTreeSet as follows:

impl<K: Ord, V> BTreeMap<K, V> {
    pub fn min_key_value(&self) -> Option<(&K, &V)>;
    pub fn max_key_value(&self) -> Option<(&K, &V)>;

    pub fn min_entry(&mut self) -> Option<OccupiedEntry<'_, K, V>>;
    pub fn max_entry(&mut self) -> Option<OccupiedEntry<'_, K, V>>;

    pub fn pop_min(&mut self) -> Option<(K, V)>;
    pub fn pop_max(&mut self) -> Option<(K, V)>;
}

impl<T: Ord> BTreeSet {
    pub fn get_min(&self) -> Option<&T>;
    pub fn get_max(&self) -> Option<&T>;

    pub fn pop_min(&mut self) -> Option<T>;
    pub fn pop_max(&mut self) -> Option<T>;
}

In my opinion min/max terminology is a lot more self-documenting and less ambiguous. You can actually see that documentation is simplified a little, because there is no need to explain what "first" and "last" mean. The fact that first/last required explanation that they actually mean min/max suggests that min/max is a better naming.

You may also notice that std's usage of BTreeSet binded values returned from these methods to variables with _min and _max, which implies that this is what the code cares about and not firstness/lastness:

-if let (Some(self_min), Some(self_max)) = (self.first(), self.last()) {
+if let (Some(self_min), Some(self_max)) = (self.get_min(), self.get_max()) {

@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 6, 2022
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Feb 25, 2022

☔ The latest upstream changes (presumably #94350) made this pull request unmergeable. Please resolve the merge conflicts.

Note: `BTreeSet::{first,last}` are renamed to `{get_min,get_max}` to
avoid conflicts with `Ord::{min,max}`.
@m-ou-se m-ou-se added I-libs-api-nominated Nominated for discussion during a libs-api team meeting. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 9, 2022
@apiraino apiraino added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 15, 2022
@bors
Copy link
Contributor

bors commented Mar 20, 2022

☔ The latest upstream changes (presumably #92962) made this pull request unmergeable. Please resolve the merge conflicts.

@m-ou-se
Copy link
Member

m-ou-se commented Apr 13, 2022

We discussed this in the libs-api meeting last week. Having to use get_min and get_max instead of min and max (because of the existing Ord::{min, max}) would be a bit inconsistent with other getter methods where we don't use the get_ prefix. Considering that, we prefer to keep the first and last names, which is also consistent with slices and other collections. It makes the ordering less explicit, but we already have the convention of using ascending order in other places, like binary_search and sort.

@m-ou-se m-ou-se removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Apr 13, 2022
@WaffleLapkin
Copy link
Member Author

WaffleLapkin commented Apr 13, 2022

That's very unfortunate, though understandable.

(and also surprising since generally inherit methods take over the trait ones (but turns out self always takes over &self))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants