Continue cleaning up collections::binary_heap
#20133
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
r? @gankro |
| /// ``` | ||
| #[unstable = "matches collection reform specification, waiting for dust to settle"] | ||
| pub fn reserve(&mut self, additional: uint) { | ||
| self.data.reserve(additional) | ||
| self.data.reserve(additional); |
There was a problem hiding this comment.
This added semicolon (as well as the one from the function just below) results in a compilation error.
There was a problem hiding this comment.
I ran make check on this and everything passed. What was the error?
There was a problem hiding this comment.
Hmm, so full disclosure, I didn't actually compile this myself, but I always get an error such as
error: not all control paths return a value
help: consider removing this semicolon
for situations like this. Though if this is not the case anymore then I stand corrected and I apologize.
Edit: Oops, the return type is () 😅 My bad, sorry!
There was a problem hiding this comment.
Every other method in this module uses a semicolon if the return type is ().
There was a problem hiding this comment.
Ah yes. You are correct, sorry!
|
Comments addressed, and rebased on master. |
| @@ -184,10 +179,10 @@ impl<T: Ord> BinaryHeap<T> { | |||
| /// | |||
| /// ``` | |||
| /// use std::collections::BinaryHeap; | |||
| /// let heap: BinaryHeap<uint> = BinaryHeap::new(); | |||
| /// let heap = BinaryHeap::<uint>::new(); | |||
There was a problem hiding this comment.
I think it's pretty rare to put type parameters of paths, and this function is conventionally called as BinaryHeap::new() rather than BinaryHeap::<T>::new(). Could this stay as it was before and perhaps have a method call after it to drive inference of what T is?
There was a problem hiding this comment.
(same comment for with_capacity below)
There was a problem hiding this comment.
er, lots of functions below!
There was a problem hiding this comment.
I'm happy to change this back, but I'm not sure which calls would be appropriate without making the example code redundant for the various methods. Any suggestions? We can also just leave it as
let heap: BinaryHeap<uint> = BinaryHeap::new();Do any of the guidelines or RFCs contain conventions for example code?
There was a problem hiding this comment.
I don't think we currently have many concrete conventions about examples, but I would expect that examples should be idiomatic code that one would expect to write under normal circumstances. I think a little duplication among examples here is ok, there's already a good bit of duplication by having one on every method, so having a push/pop seems fine.
|
Awesome, thanks @apasel422! Just a few small nits and otherwise r=me |
|
Nits addressed. |
Some more tidying up.