Add some impls of Show (RefCell, Weak, some resolve types) #19388
Conversation
|
cc #14759, #14811, #14883, #16714.
|
|
@huonw ah cool, thanks for the explanation, I'll update... |
|
updated |
|
I'd personally like to continue to push back on these impls for now. I do realize that it's super painful to not be able to use |
|
@alexcrichton I agree, we need to decide on this (my view is we want at least two traits here for debug and user facing representations), but given that this will require some design work, it is not going to happen soon. In the meantime, we removed |
|
cc @aturon |
|
We really need to make progress on the |
|
Closing in favor of rust-lang/rfcs#504 temporarily, we can reopen once the outcome of that RFC has been decided. |
|
@alexcrichton now the rfc is accepted, can we merge this? |
|
Sure! |
| write!(f, "NULL") | ||
| } else { | ||
| (*self._ptr).fmt(f) | ||
| } |
There was a problem hiding this comment.
Shouldn't this need an unsafe block? Additionally, can this use upgrade instead of duplicating logic?
There was a problem hiding this comment.
Surprisingly it does not. I used to have one in the pre-rebase version of this code, but one level of indirection disappeared and now I don't need it. I can use upgrade though.
|
r=me with mostly just using |
| @@ -719,6 +719,17 @@ impl<T> Clone for Weak<T> { | |||
| } | |||
| } | |||
|
|
|||
| #[experimental = "Show is experimental."] | |||
| impl<T: fmt::Show> fmt::Show for Weak<T> { | |||
There was a problem hiding this comment.
I'm not sure this instance is a good idea, because Weak pointers can form a cycle with Rc pointers, making fmt go into an infinite loop.
There was a problem hiding this comment.
In particular, while the RFC does state that Weak should have an implementation for Show, it might be better to just print something like (still valid Weak). As the RFC says, "implementations of the Show trait are expected to never panic! and always
produce valid UTF-8 data," and an infinite loop does not satisfy this.
An alternative to this would be using some global variable (thread_local is in std, unfortunately) to keep track of recursion depth and printing some special message if that gets too high.
There was a problem hiding this comment.
Hmm, I guess that by definition, there should be another path to an object if it is held in a weak reference, so printing a place holder is OK. Since we might expect cycles in the presence of weak pointers, this seems like a better move than going down some arbitrary depth of stack around a cycle.
r? @huonw or @alexcrichton Apparently, we have previously rejected an RFC like this. However, since then we removed `{:?}` and so without this debugging gets really difficult as soon as there is a RefCell anywhere, so I believe there is more benefit to adding these impls than there was before. By using "try_borrow" we can avoid panicing in `Show` (I think). @ huon in response to a comment in #19254: I noticed that `drop()` checks for the ptr being null, so I checked here too. Now I am checking for both, if you're confident I can change to only checking `strong()`.
r? @huonw or @alexcrichton
Apparently, we have previously rejected an RFC like this. However, since then we removed
{:?}and so without this debugging gets really difficult as soon as there is a RefCell anywhere, so I believe there is more benefit to adding these impls than there was before. By using "try_borrow" we can avoid panicing inShow(I think).@ huon in response to a comment in #19254: I noticed that
drop()checks for the ptr being null, so I checked here too. Now I am checking for both, if you're confident I can change to only checkingstrong().