Skip to content

Add size_hints for btree iterators #49550

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

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions src/liballoc/btree/map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1480,6 +1480,16 @@ impl<'a, K, V> Iterator for Range<'a, K, V> {
unsafe { Some(self.next_unchecked()) }
}
}

fn size_hint(&self) -> (usize, Option<usize>) {
if self.front == self.back {
(0, Some(0))
} else {
// There doesn't seem to be any way to get the size of the whole tree
// from a NodeRef.
(1, None)
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the utility of a size hint like this? Would it be better to just not define it at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's useful to have it, because it may be implemented in the future. size_hint is easily forgotten unless it's in the code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I buy that form of reasoning.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this particular case I buy it because (1, None) in some cases is still more information than the default size_hint of (0, None).

But #49552 contains some size_hint implementations that literally just return (0, None). For those cases I don't think I buy it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well in particular, the bit about "because it may be implemented in the future." I'd rather not add speculative impls?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BurntSushi would just adding // FIXME(#49205): Add size_hint be better? I just want to make sure it's not forgotten.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ixrec I don't know what you're talking about? None of the implementations I wrote return (0, None), or even this controversial (_, None)... Please comment on that PR if you see any specific issues, maybe you see something I don't.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other option is to add a length field like every other BTree iterator. But that isn't a trivial thing, which is what this PR is for.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just going to move forward with a FIXME. There's no consensus about what to do.

}

#[stable(feature = "map_values_mut", since = "1.10.0")]
Expand Down Expand Up @@ -1612,6 +1622,16 @@ impl<'a, K, V> Iterator for RangeMut<'a, K, V> {
unsafe { Some(self.next_unchecked()) }
}
}

fn size_hint(&self) -> (usize, Option<usize>) {
if self.front == self.back {
(0, Some(0))
} else {
// We can get the root node by using into_root_mut; but there doesn't
// seem to be any way to get the size of the tree from the root node.
(1, None)
}
}
}

impl<'a, K, V> RangeMut<'a, K, V> {
Expand Down Expand Up @@ -2549,4 +2569,19 @@ impl<K: Ord, V, I: Iterator<Item = (K, V)>> Iterator for MergeIter<K, V, I> {
}
}
}

// Currently unused. However, BTreeMap::from_sorted_iter may some day be
// rewritten to pre-allocate the correct amount of nodes, in which case
// having this will be helpful.
fn size_hint(&self) -> (usize, Option<usize>) {
let (left_lower, left_upper) = self.left.size_hint();
let (right_lower, right_upper) = self.right.size_hint();
let lower = left_lower + right_lower;
let upper = match (left_upper, right_upper) {
(Some(left), Some(right)) => left.checked_add(right),
(left, None) => left,
(None, right) => right
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None as the upper is infinity, so I think this is wrong.

Also, this lift can often be written nicely with ?-on-Option, something like

(||{ left_upper?.checked_add(right_upper?) })()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right! I got it backwards.

I don't think it's worth it to create a closure purely to use ?... it doesn't make it any more readable. IMHO a closure that is immediately called is harder to read & maintain.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it'll soon be do catch { left_upper?.checked_add(right_upper?)? } to avoid the IIFE (that I agree is ugly), but please don't do that until #49371 merges.

Copy link
Contributor Author

@Phlosioneer Phlosioneer Apr 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's still a lot of ?. But much better. I'll be sure to make that change whenever it merges.

};
(lower, upper)
}
}
4 changes: 4 additions & 0 deletions src/liballoc/btree/set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -987,6 +987,10 @@ impl<'a, T> Iterator for Range<'a, T> {
fn next(&mut self) -> Option<&'a T> {
self.iter.next().map(|(k, _)| k)
}

fn size_hint(&self) -> (usize, Option<usize>) {
self.iter.size_hint()
}
}

#[stable(feature = "btree_range", since = "1.17.0")]
Expand Down