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

proposal for BTreeMap/Set min/max, #62924 #65637

Merged
merged 1 commit into from Nov 13, 2019

Conversation

@ssomers
Copy link
Contributor

ssomers commented Oct 20, 2019

  • Which pair of names: #62924 lists the existing possibilities min/max, first/last, (EDIT) front/back, peek(/peek_back?). Iterators have next/next_back or next/last. I'm slightly in favour of first/last because min/max might suggest they search over the entire map, and front/back pretends they are only about position.
  • Return key only instead of pair like iterator does?
  • If not, then keep the _key_value suffix? Also provide variant with mutable value? But there is no such variant for get_key_value.
  • Look for and upgrade more usages of .iter().next() and such in the libraries? I only upgraded the ones I contributed myself, all very recently.
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Oct 20, 2019

r? @kennytm

(rust_highfive has picked a reviewer for you, use r? to override)

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Oct 20, 2019

@rust-highfive rust-highfive assigned bluss and unassigned kennytm Oct 20, 2019
@scottmcm scottmcm added the T-libs label Oct 21, 2019
@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Oct 21, 2019

Hmm, get vs get_key_value definitely makes this awkward to decide.

Slightly crazy idea: first_entry() -> Option<OccupiedEntry<K, V>>. Then you can even use remove_entry() on it to remove the first pair without needing to copy the key to be able to use it to lookup the item again in order to remove it. (Though it has the obvious downside of needing &mut.)

@ssomers

This comment has been minimized.

Copy link
Contributor Author

ssomers commented Oct 21, 2019

I'm definitely crazy about first_entry, but I'll try to restrain from sneaking a BTreeSet::pop_front into this PR.

@ssomers ssomers force-pushed the ssomers:master branch from c11fb03 to 95967f7 Oct 22, 2019
@ssomers

This comment has been minimized.

Copy link
Contributor Author

ssomers commented Oct 22, 2019

Restrain myself? I'll leave that up to others. Let's pop ahead of HashMap users and make BTreeSet even faster than it already is!

}

/// Removes the first value from the set and returns it.
/// The first value is always the minmumn value in the set.

This comment has been minimized.

Copy link
@kennytm

kennytm Oct 23, 2019

Member
Suggested change
/// The first value is always the minmumn value in the set.
/// The first value is always the minimum value in the set.
@ssomers ssomers force-pushed the ssomers:master branch from 95967f7 to ffeac1f Oct 23, 2019
@ssomers

This comment has been minimized.

Copy link
Contributor Author

ssomers commented Oct 23, 2019

I noticed that in my 2nd commit, but also in the earliest documentation commit 21ac985, some of the descriptions of BTreeSet mention "element" and some "value" for the thingies contained. I settled on "value" for the new functions because that seems most popular (8 against 6, discarding similar descriptions and those I contributed).

Also, a little more proper unit testing (separate from doc tests).

@bluss

This comment has been minimized.

Copy link
Member

bluss commented Oct 30, 2019

I'm sorry, I haven't had time to review, still expect to do it, but don't mind someone else jumping in.

@ssomers

This comment has been minimized.

Copy link
Contributor Author

ssomers commented Oct 31, 2019

Here are some arguments for and against the immutable variants:

  • Yes: Performance (#62924)
  • No: the optimizer should eliminate the overhead of .iter().next() mocking about in a temporary.
  • Yes: Making it clear that min/max is cheaply available. It took me a while to figure this out earlier. Also because the description of BTreeMap mentions key order only in passing ("sorted map" buried in implementation detail), though individual functions are clear ("sorted by key", "in sorted order", "in ascending order"...)
  • No: in C++ neither std::map nor std::set have them - you need !m.empty() ? *m.begin() : something_else and *m.rbegin(). Though that leads back to #62924: C++ iterators are better at this.
  • Yes: in Java, interface SortedMap has firstKey/lastKey and interface SortedSet has first/last

For and against the mutable variants:

  • Yes: promote BTreeMap as a sophisticated priority queue. BinaryHeap does not allow removing entries other than popping from one end, as far as I understand.
  • No: in C++ neither std::map nor set::set have them - you need m.erase(m.begin()) and m.erase(m.rbegin().base())
  • Yes: C++ equivalent is much faster because the only way to erase a map entry in Rust involves a key lookup, it seems.
  • No: Rust's equivalent should be as fast. BTreeMap::iter_mut should relate to OccupiedEntry.
  • Yes: in Java, implementation TreeMap has pollFirstEntry/pollLastEntry and implementation TreeSet has pollFirst/pollLast
  • Yes: BTreeSet doesn't have BTreeMap's mutable iterators or entries, nor can I imagine a reasonable use for them other than removing first or last from the set (i.e. removing a given key if it exists is already covered by remove). So I have a hard time imagining any reasonable alternative for the two BTreeSet functions (apart from their name).
  • Yes: people do mention a desire for such functions, e.g. RFC 1800, #27804 both mention pop_min/pop_max.
@bluss

This comment has been minimized.

Copy link
Member

bluss commented Nov 6, 2019

@Centril can I reassign this? I won't have the time unfortunately

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Nov 6, 2019

Sure, let's try r? @scottmcm

@rust-highfive rust-highfive assigned scottmcm and unassigned bluss Nov 6, 2019
@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Nov 12, 2019

These seems plausible and the implementations look fine. I think it's ok for them to get checked in, and then libs can decide later if they're worth stabilizing or should be removed.

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 12, 2019

📌 Commit ffeac1f has been approved by scottmcm

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 13, 2019

⌛️ Testing commit ffeac1f with merge 374ad1b...

bors added a commit that referenced this pull request Nov 13, 2019
proposal for BTreeMap/Set min/max, #62924

- Which pair of names: #62924 lists the existing possibilities min/max, first/last, (EDIT) front/back, peek(/peek_back?). Iterators have next/next_back or next/last. I'm slightly in favour of first/last because min/max might suggest they search over the entire map, and front/back pretends they are only about position.
- Return key only instead of pair like iterator does?
- If not, then keep the _key_value suffix? ~~Also provide variant with mutable value? But there is no such variant for get_key_value.~~
- Look for and upgrade more usages of `.iter().next()` and such in the libraries? I only upgraded the ones I contributed myself, all very recently.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 13, 2019

☀️ Test successful - checks-azure
Approved by: scottmcm
Pushing 374ad1b to master...

@bors bors added the merged-by-bors label Nov 13, 2019
@bors bors merged commit ffeac1f into rust-lang:master Nov 13, 2019
5 checks passed
5 checks passed
homu Test successful
Details
pr Build #20191023.50 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
ssomers added a commit to ssomers/rust_bench_sets_compared that referenced this pull request Nov 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.