BTreeMap: comment why drain_filter's size_hint is somewhat pessimistic #77449
Conversation
rust-highfive
added
the
S-waiting-on-review
jyn514
added
the
T-libs
|
I am a bit confused -- we're updating the length live, so why do we need a new remaining field? My guess is that there's something subtle going on here that would be good to document on the remaining field (as well as clarify here). If we do need the additional field I think we shouldn't do this, it's not worth the extra field and more complicated logic. |
Mark-Simulacrum
added
S-waiting-on-author
|
The length includes the part of the map that the predicate saw and decided not to drain. It overestimates how many elements are yet to be returned by the iterator. I agree this is rather futile. If the predicate keeps very many elements, then the size hint is always overestimated. If the predicate drains very many elements, then you're better of with |
|
I think it's probably fine for us to include that extra in the size hint -- it's a hint, and I'd prefer to avoid the complication introduced by the logic here (I am surprised tracking the information we need requires so many changes). I am leaning towards saying that we shouldn't do this. |
Well it doesn't require changes in navigate.rs, but it allows getting rid of some Err values and saving a total of 5 lines there. But without really reducing complexity. I'll write a comment in the size_hint function instead. |
ssomers
force-pushed
the
btree_drain_filter_size_hint
branch
from
ab4cb00 to
3b051d0
Compare
ssomers
changed the title
|
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author |
rustbot
added
S-waiting-on-review
Mark-Simulacrum
changed the title
|
@bors r+ rollup |
|
📌 Commit 3b051d0 has been approved by |
bors
added
S-waiting-on-bors
…t, r=Mark-Simulacrum BTreeMap: comment why drain_filter's size_hint is somewhat pessimistic The `size_hint` of the `DrainFilter` iterator doesn't adjust as you iterate. This hardly seems important to me, but there has been a comparable PR rust-lang#64383 in the past. I guess a scenario is that you first iterate half the map manually and keep most of the key/value pairs in the map, and then tell the predicate to drain most of the key/value pairs and `.collect` the iterator over the remaining half of the map. I am totally ambivalent whether this is better or not. r? @Mark-Simulacrum
…as-schievink Rollup of 8 pull requests Successful merges: - rust-lang#76750 (Don't discourage implementing `core::fmt::Write`) - rust-lang#77449 (BTreeMap: comment why drain_filter's size_hint is somewhat pessimistic) - rust-lang#77660 ((docs): make mutex error comment consistent with codebase) - rust-lang#77663 (Add compile fail test for issue 27675) - rust-lang#77673 (Remove unnecessary lamda on emitter map.) - rust-lang#77701 (Make `max_log_info` easily greppable (for figuring out why debug logging is disabled)) - rust-lang#77702 (Remove not needed lambda.) - rust-lang#77710 (Update submodule llvm to get LVI bugfix) Failed merges: r? `@ghost`
The
size_hintof theDrainFilteriterator doesn't adjust as you iterate. This hardly seems important to me, but there has been a comparable PR #64383 in the past. I guess a scenario is that you first iterate half the map manually and keep most of the key/value pairs in the map, and then tell the predicate to drain most of the key/value pairs and.collectthe iterator over the remaining half of the map.I am totally ambivalent whether this is better or not.
r? @Mark-Simulacrum