-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
Remove implicit shrinking from hash_map. #18770
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
Conversation
|
Preferably the |
f0d54cf to
3bdd519
Compare
|
Done. It's a little unintuitive that a breaking-change probably won't break any code. |
3bdd519 to
ef75c37
Compare
src/libstd/collections/hash/map.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just drop "the range" here
|
All the actual implementation logic looks good to me. Just some doc nits, and some issues about the precision we can assume for resize in testing. |
|
Oh! Hashset's capacity stuff is probably all wrong now. Probably best to fix that up in this PR too? (should just be doc, maybe test changes) |
dc7c5cf to
31ec9ee
Compare
|
This all looks basically good to me now. Just need a real reviewer to take a look. Might want to update some tests to reflect newer semantics but it's not a big deal. Also might want to wait for @aturon to make a proclamation about how we want to map r? @huonw |
e6da055 to
f10cac8
Compare
This is breaking code: it will not stop things from compiling, but it is dramatically changing semantics and could, e.g. cause code that previously worked fine to run out of memory ( |
src/libstd/collections/hash/map.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this handle arithmetic overflow correctly?
Usable capacity, for sure. Everything else is an implementation detail that should be hidden. If we ever want to be able to use |
|
Updated with usable capacity |
a0764ef to
2f4f753
Compare
|
Whoops, this slipped through the cracks. I believe all of my issues were addressed. @huonw anything outstanding for you? |
src/libstd/collections/hash/map.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the required property just min_capacity(usable_capacity(x)) <= x? Because this is always true for positive x: integer division is floored, not just some unspecified rounding, i.e. (cap / 11) * 11 <= cap always.
(I'm just raising this because that property means this comment is slightly confusing to me, as it is either redundant or talking about something else entirely.)
|
Done. Hopefully I got capacity equality right. |
11dc54e to
7564168
Compare
Implements fn shrink_to_fit for HashMap.
HashMap's `reserve` method now takes as an argument the *extra* space to reserve. [breaking-change]
7564168 to
b82624b
Compare
Part of enforcing capacity-related conventions, for #18424, the collections reform. Implements `fn shrink_to_fit` for HashMap. The `reserve` method now takes as an argument the *extra* space to reserve.
Part of enforcing capacity-related conventions, for #18424, the collections reform.
Implements
fn shrink_to_fitfor HashMap.The
reservemethod now takes as an argument the extra space to reserve.