authorgravatar for squeek502@hotmail.comRyan Liptak <squeek502@hotmail.com> 2019-04-06 14:15:12-07:00
committergravatar for andrew@ziglang.orgAndrew Kelley <andrew@ziglang.org> 2019-04-07 15:47:09-04:00
log6a78b315b2112e372b48ba7399ea9cddadbe65b6
tree4d196cae30369468c1912139e6f8a7f720d45913
parent6715c54cc641e61405bb72d286fe2cf560447b56

Fix std.HashMap.remove returning incorrect KV

Now returns a copy of the removed kv instead of a pointer to the removed kv. The removed kv gets overwritten when shifting the hash map after the removal, so returning a pointer to it will have another kv's values in it after the return. This bug had some nasty downstream effects in things like BufSet and BufMap where delete would free a still in-use KV and leave the actually removed KV un-free'd.

1 files changed, 7 insertions(+), 3 deletions(-)

std/hash_map.zig+7-3
...@@ -175,7 +175,7 @@ pub fn HashMap(comptime K: type, comptime V: type, comptime hash: fn (key: K) u3...@@ -175,7 +175,7 @@ pub fn HashMap(comptime K: type, comptime V: type, comptime hash: fn (key: K) u3
175 return hm.get(key) != null;175 return hm.get(key) != null;
176 }176 }
177177
178 pub fn remove(hm: *Self, key: K) ?*KV {178 pub fn remove(hm: *Self, key: K) ?KV {
179 if (hm.entries.len == 0) return null;179 if (hm.entries.len == 0) return null;
180 hm.incrementModificationCount();180 hm.incrementModificationCount();
181 const start_index = hm.keyToIndex(key);181 const start_index = hm.keyToIndex(key);
...@@ -189,13 +189,14 @@ pub fn HashMap(comptime K: type, comptime V: type, comptime hash: fn (key: K) u3...@@ -189,13 +189,14 @@ pub fn HashMap(comptime K: type, comptime V: type, comptime hash: fn (key: K) u3
189189
190 if (!eql(entry.kv.key, key)) continue;190 if (!eql(entry.kv.key, key)) continue;
191191
192 const removed_kv = entry.kv;
192 while (roll_over < hm.entries.len) : (roll_over += 1) {193 while (roll_over < hm.entries.len) : (roll_over += 1) {
193 const next_index = (start_index + roll_over + 1) % hm.entries.len;194 const next_index = (start_index + roll_over + 1) % hm.entries.len;
194 const next_entry = &hm.entries[next_index];195 const next_entry = &hm.entries[next_index];
195 if (!next_entry.used or next_entry.distance_from_start_index == 0) {196 if (!next_entry.used or next_entry.distance_from_start_index == 0) {
196 entry.used = false;197 entry.used = false;
197 hm.size -= 1;198 hm.size -= 1;
198 return &entry.kv;199 return removed_kv;
199 }200 }
200 entry.* = next_entry.*;201 entry.* = next_entry.*;
201 entry.distance_from_start_index -= 1;202 entry.distance_from_start_index -= 1;
...@@ -371,7 +372,10 @@ test "basic hash map usage" {...@@ -371,7 +372,10 @@ test "basic hash map usage" {
371372
372 testing.expect(map.contains(2));373 testing.expect(map.contains(2));
373 testing.expect(map.get(2).?.value == 22);374 testing.expect(map.get(2).?.value == 22);
374 _ = map.remove(2);375
376 const rmv1 = map.remove(2);
377 testing.expect(rmv1.?.key == 2);
378 testing.expect(rmv1.?.value == 22);
375 testing.expect(map.remove(2) == null);379 testing.expect(map.remove(2) == null);
376 testing.expect(map.get(2) == null);380 testing.expect(map.get(2) == null);
377}381}