proposal for BTreeMap/Set min/max, #62924

[ssomers]

• Which pair of names: Tracking issue for map_first_last: first/last methods on BTreeSet and BTreeMap #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. PS see also RFC 1195.
• 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]

r? @kennytm

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

[Centril]

r? @bluss

cc @scottmcm @dtolnay

[scottmcm]

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]

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

[ssomers]

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!

[kennytm]

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]

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]

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

[ssomers]

Here are some arguments for and against the immutable variants:

• Yes: Performance (Tracking issue for map_first_last: first/last methods on BTreeSet and BTreeMap #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 Tracking issue for map_first_last: first/last methods on BTreeSet and BTreeMap #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, add pop() to HashSet etc.? #27804 both mention pop_min/pop_max.

[bluss]

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

[Centril]

Sure, let's try r? @scottmcm

[scottmcm]

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]

📌 Commit ffeac1f has been approved by scottmcm

[bors]

⌛ Testing commit ffeac1f with merge 374ad1b...

[bors]

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