Uh oh!
There was an error while loading. Please reload this page.
Add Debug implementations for libcollection structs - #39002
Conversation
There was a problem hiding this comment.
What's the plan here about showing the elements? It needs specialization to add the case for T: fmt::Debug later, and I don't think we want the complexity of having both impls (without debug bound and with) for each struct.
There was a problem hiding this comment.
I kept the implementation as easy as possible. Also, why would it need an implementation for T: fmt::Debug? Except if you want to see the internals, I'm not sure this is very useful.
There was a problem hiding this comment.
To identify the iterator with its sequence of elements seems like the logical thing (if it is easy to iterate the sequence from &self). Like how [1, 2, 3].iter() has a debug that shows Iter([1, 2, 3])
There was a problem hiding this comment.
In this case, it'd require to consume self in order to see the values. I don't think we want this behavior.
There was a problem hiding this comment.
Why do you have to consume? Can't you iterate over self.iter.as_slice()?
There was a problem hiding this comment.
It doesn't have such a method as far as I can tell, or maybe I missed it?
There was a problem hiding this comment.
/// `BinaryHeap` iterator.#[stable(feature = "rust1", since = "1.0.0")]pubstructIter<'a,T:'a>{iter: slice::Iter<'a,T>,}https://doc.rust-lang.org/std/slice/struct.Iter.html#method.as_slice
9e29d1a to
5b06afdCompareGuillaumeGomez
commented
Jan 16, 2017
Updated. |
frewsxcv
commented
Jan 16, 2017
In my opinion, every |
GuillaumeGomez
commented
Jan 16, 2017
I'll add others as well. |
640d50b to
5dda7f7CompareHere's the |
5dda7f7 to
30691b9CompareGuillaumeGomez
commented
Jan 18, 2017
Updated. No more commented impl. |
alexcrichton
commented
Jan 19, 2017
Why is specialization being used here? Is that being used elsewhere in |
FWIW, specialization is being used in the other PR you approved: #39156 Also, |
GuillaumeGomez
commented
Jan 19, 2017
@alexcrichton: Is it an issue? Sounded like a good idea to me... |
alexcrichton
commented
Jan 19, 2017
@GuillaumeGomez yes we don't use specialization for these purposes, just performance in a few minor scenarios. These types should likely all require that generics all implement |
GuillaumeGomez
commented
Jan 20, 2017
I removed specialization. |
There was a problem hiding this comment.
This should use the debug builders instead of allocating a string.
There was a problem hiding this comment.
Same as above with string allocation (and a number of instances below too)
25258cd to
513532eCompareGuillaumeGomez
commented
Jan 20, 2017
Updated (for the two given). |
513532e to
668af80CompareGuillaumeGomez
commented
Jan 20, 2017
A new struct has been added since I started this PR, it has its debug implementation as well. |
| .field(&self.vec.as_slice()) | ||
| .finish() | ||
| } | ||
| } |
There was a problem hiding this comment.
How come implementations like this aren't using #{derive(Debug)]?
There was a problem hiding this comment.
The habit, didn't thought of testing it, my bad... However, once this PR merged, such cases won't happen again thanks to the corresponding deny option. ;)
| #[stable(feature = "collection_debug", since = "1.15.0")] | ||
| impl<T: fmt::Debug> fmt::Debug for IntoIter<T> { | ||
| fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
| f.pad(&format!("BTreeSet::IntoIter({:?})", self.iter)) |
There was a problem hiding this comment.
There's still intermediate strings here (and in other places)
There was a problem hiding this comment.
Ok, I'll do a pass for the missing ones. Sorry about this.
GuillaumeGomez
commented
Jan 20, 2017
I removed all the |
There was a problem hiding this comment.
To match the others, should this not be "BinaryHeap::PeekMut"? Although it might be better to use part of the actual path like "binary_heap::PeekMut" for all of these instead.
There was a problem hiding this comment.
I prefer to refer to the struct used to create the iterator rather than the path of the iterator.
There was a problem hiding this comment.
BinaryHeapPlace is unstable so this impl should get the same stability attribute as BinaryHeapPlace.
There was a problem hiding this comment.
EnumSet is unstable so this stability attribute should probably be removed.
d62f2b8 to
8daacb3CompareGuillaumeGomez
commented
Jan 23, 2017
@ollie27: Thanks for your review! I think I fixed all of them (replacing |
There was a problem hiding this comment.
Currently the precedence for this is to not have the namespace prefix, so let's remove it.
There was a problem hiding this comment.
This should also just be #[derive]
Note that when I point things out like this it's typically useful to go over the PR looking for other cases as well. I'll try to exhaustively point it out this time but only fixing precisely what is pointed out isn't always the best strategy.
There was a problem hiding this comment.
This is probably infinite recursion
b6dd3f1 to
393c3e4CompareGuillaumeGomez
commented
Jan 23, 2017
@alexcrichton: I think I addressed all your review comments. |
alexcrichton
commented
Jan 24, 2017
@rfcbot fcp merge |
Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams: No concerns currently listed. Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
alexcrichton
commented
Feb 4, 2017
There was a problem hiding this comment.
I guess this missed 1.16 so these will need changing to 1.17 now.
393c3e4 to
0cc2448CompareGuillaumeGomez
commented
Feb 5, 2017
Updated the release version number to 1.17. |
GuillaumeGomez
commented
Feb 7, 2017
ping @brson |
aturon
commented
Feb 7, 2017
@bors: r+ |
bors
commented
Feb 7, 2017
📌 Commit 0cc2448 has been approved by |
bors
commented
Feb 7, 2017
Add Debug implementations for libcollection structs Part of #31869.
bors
commented
Feb 7, 2017
☀️ Test successful - status-appveyor, status-travis |
Part of #31869.