-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Remove implicit shrinking from hash_map. #18770
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
fn capacity_range(&self, new_size: uint) -> (uint, uint) { | ||
// Here, we are rephrasing the logic by specifying the ranges: | ||
fn min_capacity(&self, new_size: uint) -> uint { | ||
// Here, we are rephrasing the logic by specifying the range: |
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 ( |
let cap = num::next_power_of_two( | ||
max(INITIAL_CAPACITY, new_minimum_capacity)); | ||
#[unstable = "matches collection reform specification, waiting for dust to settle"] | ||
pub fn reserve(&mut self, additional: uint) { |
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? |
fn reserve(&mut self, new_capacity: uint) { | ||
self.minimum_capacity2 = new_capacity << 1; | ||
fn usable_capacity(&self, cap: uint) -> uint { | ||
// Integer division with rounding is inexact. However, |
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_fit
for HashMap.The
reserve
method now takes as an argument the extra space to reserve.