add `Show` impl for Tree{Map,Set} and cleanup #14447

erickt · 2014-05-26T22:33:16Z

This is a hodge podge of a couple small cleanup commits. It implements Show for TreeMap and TreeSet, and some removal of commented out code.

sfackler · 2014-05-26T22:37:24Z

src/libserialize/json.rs

-                        res.push_char(char::from_u32(n as u32).unwrap());
-                    }
-                    _ => return self.error(InvalidEscape),
->>>>>>> Add a streaming parser to serialize::json.*/


sfackler · 2014-05-26T22:38:27Z

Should that commented block be deleted or uncommented?

lilyball · 2014-05-26T22:38:28Z

Is there an actual need for reserve_additional() here? In a Vec it makes sense, but in a HashMap reserve() actually has different behavior, which is to say, it establishes a minimum capacity that must be maintained, as opposed to merely establishing the current capacity.

lilyball · 2014-05-26T22:41:04Z

src/libcollections/hashmap.rs

+    /// Reserve space for an additional `n` elements in the hash table.
+    pub fn reserve_additional(&mut self, extra: uint) {
+        let len = self.len();
+        if self.minimum_capacity - len < extra {


minimum_capacity is not guaranteed to be >= len. It's a minimum and is only modified by an explicit call to reserve() or, apparently, to clear().

lilyball · 2014-05-26T22:42:55Z

Thinking about it some more, it seems to me that reserve_additional() is basically just used as a way of saying "I'm about to add n new elements, don't reallocate repeatedly". Given that reserve() actually modifies a minimum capacity instead of just modifying the current capacity, perhaps reserve_additional() should not be a wrapper around reserve(), but instead it should do whatever is appropriate to grow the HashMap to the capacity it would have if it had len + n elements, without modifying minimum_capacity.

I'm assuming here that HashMap never shrinks when new elements are added, but only when elements are deleted, but you'd have to verify this.

erickt · 2014-05-26T22:44:19Z

@sfackler: parsing of escaped characters was replaced with https://github.com/mozilla/rust/blob/master/src/libserialize/json.rs#L1411 in #13469, which fixed decoding non-BMP hex escapes, so I think it should be okay to remove this dead code. It just got missed in a review.

erickt · 2014-05-26T23:10:25Z

@kballard: Yep, I added it because I wanted to add a bunch of items to an already existing HashMap. In regards to if reserve_additional() should be wrapping reserve(), I think how I'm doing it is correct. This is the same semantics as Vec::reserve_additional(), where reserve_additional is just doing a overflow-checked version of v.reserve(v.len() + extra), so it makes since that the minimum_capacity would be changed here. If you want to resize a HashMap without changing the minimum_capacity, then I feel that should probably be a different name than .reserve_* prefix.

lilyball · 2014-05-26T23:11:43Z

@erickt Vec doesn't have a concept of minimum_capacity. It just has the concept of current capacity, and reserve_additional() bumps the current capacity. Vec also never shrinks without a call to shrink_to_fit() (AFAIK).

HashMap not only has minimum_capacity but I believe it will shrink automatically (down to the minimum_capacity) as it's modified.

lilyball · 2014-05-26T23:13:07Z

Basically, reserve() on HashMap appears to be a call that you use to tune its behavior based on the expected number of elements it should have in the normal case. reserve_additional() on the other hand is basically a method that says "I'm going to be adding a bunch of items, make sure you have capacity up-front for them". I don't think the latter should be touching minimum_capacity.

Given the difference of reserve() here compared to Vec, perhaps we should actually change the name of reserve().

erickt · 2014-05-26T23:28:32Z

@kballard: I still don't see the issue here :) In my opinion, the .reserve_additional() methods are really supposed to be a safe way to do x.reserve(x.len() + 1) without having to sprinkle .checked_add()s all over the place. I suppose we could change things around where .reserve()/.reserve_additional() don't touch minimum_capacity, and .resize/.resize_additional() do touch it, it's still convenient in both cases to have a *_additional() method variant. I'm not ready to do a PR to change the semantics of .reserve() now though as I don't understand the implications of doing that, so it should probably be left for a future PR or IRC conversation.

lilyball · 2014-05-27T00:00:46Z

I think reserve_additional() makes sense if we change the semantics of reserve(), but I think with the current semantics of reserve(), having reserve_additional() gives the wrong impression of what the method is for. Because it's not for telling the HashMap that you're about to add 17 new elements (because telling the HashMap that doesn't tell it anything about the expected average usage over the lifetime of the HashMap).

Given that, I would feel more comfortable removing reserve_additional() from this PR and instead having a separate PR that changes the name of reserve() to something that indicates it's for setting the minimum capacity. Then a new reserve() can be written that really just ensures the current capacity is large enough, and reserve_additional() then makes sense in light of that.

erickt · 2014-05-27T17:32:01Z

@kballard: I removed the reserve_additional() patch so it doesn't hold up the rest of this PR. I'll move it into another PR so we can continue the discussion there.

lilyball · 2014-05-27T17:36:44Z

src/libcollections/treemap.rs

@@ -67,6 +69,19 @@ impl<K: Ord + TotalOrd, V: Ord> Ord for TreeMap<K, V> {
    fn lt(&self, other: &TreeMap<K, V>) -> bool { lt(self, other) }
 }

+impl<K: Eq + TotalOrd + Show, V: Show> Show for TreeMap<K, V> {


Eq is an unnecessary bound here. TreeMap only requires TotalOrd (which as it happens includes Eq via TotalEq).

I'm not convinced that we need extra bounds (like TotalOrd) at all on unrelated traits, since you can't even construct a TreeMap without TotalOrd, but precedent so far is to use the "required" bounds on all traits. So <K: TotalOrd + Show, V: Show> should be correct.

lilyball · 2014-05-27T17:39:07Z

src/libcollections/treemap.rs

@@ -547,6 +562,19 @@ impl<T: Ord + TotalOrd> Ord for TreeSet<T> {
    fn lt(&self, other: &TreeSet<T>) -> bool { self.map < other.map }
 }

+impl<T: Eq + TotalOrd + Show> Show for TreeSet<T> {


Same thing about the Eq bound here.

lilyball · 2014-05-27T17:39:51Z

r=me with comments

erickt · 2014-05-27T17:41:07Z

@kballard: I addressed your comments in the latest version.

This is a hodge podge of a couple small cleanup commits. It implements `Show` for `TreeMap` and `TreeSet`, and some removal of commented out code.

sfackler reviewed May 26, 2014
View reviewed changes

lilyball reviewed May 26, 2014
View reviewed changes

erickt added 2 commits May 27, 2014 10:20

serialize: Remove old commented out code

85d9cfb

collections: add Show impl test for HashMap

e9e799f

erickt changed the title ~~add Show impl for Tree{Map,Set}, add .reserve_additional, and cleanup~~ add Show impl for Tree{Map,Set} and cleanup May 27, 2014

lilyball reviewed May 27, 2014
View reviewed changes

collections: implement Show for Tree{Map,Set}

926504c

lilyball reviewed May 27, 2014
View reviewed changes

bors added a commit that referenced this pull request May 27, 2014

auto merge of #14447 : erickt/rust/show-treemap, r=kballard

30bf73f

This is a hodge podge of a couple small cleanup commits. It implements `Show` for `TreeMap` and `TreeSet`, and some removal of commented out code.

bors closed this May 28, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `Show` impl for Tree{Map,Set} and cleanup #14447

add `Show` impl for Tree{Map,Set} and cleanup #14447

erickt commented May 26, 2014

sfackler May 26, 2014

sfackler commented May 26, 2014

lilyball commented May 26, 2014

lilyball May 26, 2014

lilyball commented May 26, 2014

erickt commented May 26, 2014

erickt commented May 26, 2014

lilyball commented May 26, 2014

lilyball commented May 26, 2014

erickt commented May 26, 2014

lilyball commented May 27, 2014

erickt commented May 27, 2014

lilyball May 27, 2014

lilyball May 27, 2014

lilyball commented May 27, 2014

erickt commented May 27, 2014

add Show impl for Tree{Map,Set} and cleanup #14447

add Show impl for Tree{Map,Set} and cleanup #14447

Conversation

erickt commented May 26, 2014

sfackler May 26, 2014

Choose a reason for hiding this comment

sfackler commented May 26, 2014

lilyball commented May 26, 2014

lilyball May 26, 2014

Choose a reason for hiding this comment

lilyball commented May 26, 2014

erickt commented May 26, 2014

erickt commented May 26, 2014

lilyball commented May 26, 2014

lilyball commented May 26, 2014

erickt commented May 26, 2014

lilyball commented May 27, 2014

erickt commented May 27, 2014

lilyball May 27, 2014

Choose a reason for hiding this comment

lilyball May 27, 2014

Choose a reason for hiding this comment

lilyball commented May 27, 2014

erickt commented May 27, 2014

add `Show` impl for Tree{Map,Set} and cleanup #14447

add `Show` impl for Tree{Map,Set} and cleanup #14447