Ensure Guard types impl Display & Debug #42822

ChrisMacNaughton · 2017-06-22T10:04:46Z

rust-highfive · 2017-06-22T10:05:01Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Mark-Simulacrum · 2017-06-22T11:46:14Z

src/libstd/sync/mutex.rs

@@ -393,6 +393,19 @@ impl<T: ?Sized + fmt::Debug> fmt::Debug for Mutex<T> {
    }
 }

+#[unstable(feature = "std_display", issue = "24372")]


All of these can be made #[stable(feature = "std_guard_impls", since = "1.20")] since stability on impls isn't tracked today. I think these will go into 1.20, but I could be wrong.

I wasn't sure about the stability markers but I knew it would need something, thanks!

frewsxcv · 2017-06-22T13:05:40Z

src/libstd/sync/rwlock.rs

+impl<T: ?Sized + fmt::Display> fmt::Display for RwLock<T> {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self.try_read() {
+            Ok(guard) => write!(f, "RwLock {{ data: {} }}", guard),


Is this supposed to be for Display or Debug? This formatting suggests it's for Debugbut this is animplforDisplay`

I'm not 100% satisfied with this representation for Display, I'm just not sure how it should be given that we could be unable to effectively lock the guard and get at the internal data

frewsxcv · 2017-06-22T13:06:49Z

Weird that these are supposedly missing Debug impls, since we have #![deny(missing_debug_implementations)] at the libcore root

alexcrichton

Looking great, thanks @ChrisMacNaughton!

alexcrichton · 2017-06-22T14:31:09Z

src/libstd/sync/mutex.rs

@@ -393,6 +393,19 @@ impl<T: ?Sized + fmt::Debug> fmt::Debug for Mutex<T> {
    }
 }

+#[stable(feature = "std_guard_impls", since = "1.20")]
+impl<T: ?Sized + fmt::Display> fmt::Display for Mutex< T> {


We tend to prefer Display for only values that can be losslessy displayed, so perhaps this could be removed?

Without Mutex being Display, it's a lot more difficult for MutexGuard to be Display as self.__lock can't be Displayed

alexcrichton · 2017-06-22T14:31:29Z

src/libstd/sync/mutex.rs

+#[stable(feature = "std_guard_impls", since = "1.20")]
+impl<'a, T: ?Sized + fmt::Display> fmt::Display for MutexGuard<'a, T> {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "{}", self.__lock)


Could this use * along the lines of (**self).fmt(f) to do the displaying?

alexcrichton · 2017-06-22T14:31:49Z

src/libcore/cell.rs

+#[stable(feature = "std_guard_impls", since = "1.20")]
+impl<'a, T: ?Sized + fmt::Display> fmt::Display for Ref<'a, T> {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "{}", self.value)


These might be best implemented as self.value.fmt(f) as that'll forward formatting flags and such

alexcrichton · 2017-06-22T14:32:02Z

src/libstd/sync/rwlock.rs

@@ -330,6 +330,19 @@ impl<T: ?Sized + fmt::Debug> fmt::Debug for RwLock<T> {
    }
 }

+#[stable(feature = "std_guard_impls", since = "1.20")]
+impl<T: ?Sized + fmt::Display> fmt::Display for RwLock<T> {


Like with Mutex above I think it's ok to remove this one.

Mark-Simulacrum · 2017-06-22T14:49:19Z

I think this is almost ready, if you want to squash commits (I can provide instructions if you'd like) and edit the description so it states Fixes #xxx, that way GitHub will automatically close the issue once this merges.

Fixes rust-lang#24372

ChrisMacNaughton · 2017-06-22T14:56:05Z

@Mark-Simulacrum Does the PR need to have the Fixes: ### as well, or is it fine for just the commit as now?

Mark-Simulacrum · 2017-06-22T14:57:23Z

I believe the PR description needs to have it; you should be able to edit it.

ChrisMacNaughton · 2017-06-22T15:01:47Z

@Mark-Simulacrum I can't seem to update the PR's description, but #24372 (reference) that the issue will close when this PR is merged from the commit message.

sfackler · 2017-06-22T15:04:01Z

I fixed it up for you!

ChrisMacNaughton · 2017-06-22T15:05:45Z

thanks @sfackler 😂 Didn't realize that the first comment ends up being the PR description

alexcrichton · 2017-06-22T15:26:50Z

@bors: r+

Thanks @ChrisMacNaughton!

bors · 2017-06-22T15:26:51Z

📌 Commit 14df549 has been approved by alexcrichton

Mark-Simulacrum · 2017-06-22T23:17:18Z

@bors rollup

…excrichton Ensure Guard types impl Display & Debug Fixes rust-lang#24372

Rollup of 8 pull requests - Successful merges: #42777, #42783, #42787, #42821, #42822, #42825, #42829, #42833 - Failed merges:

…r=dtolnay Add T: ?Sized to `RwLockReadGuard` and `RwLockWriteGuard`'s Debug impls. For context, `MutexGuard` has `+ ?Sized` on its `Debug` impl, and all three have `+ ?Sized` on their `Display` impls. It looks like the `?Sized` was just missed when the impls were added (the impl for `MutexGuard` was added in the same PR (rust-lang#38006) with support for `T: Debug + ?Sized`, and `RwLock*Guard`s did allow `T: ?Sized` types already); the `Display` impls were added later (rust-lang#42822) with support for `T: Debug + ?Sized` types. I think this needs a T-libs-api FCP? I'm not sure if this also needs an ACP. If so I can make one. These are changes to (stable) trait impls on stable types so will be insta-stable. `@rustbot` label +T-libs-api

Add T: ?Sized to `RwLockReadGuard` and `RwLockWriteGuard`'s Debug impls. For context, `MutexGuard` has `+ ?Sized` on its `Debug` impl, and all three have `+ ?Sized` on their `Display` impls. It looks like the `?Sized` was just missed when the impls were added (the impl for `MutexGuard` was added in the same PR (rust-lang/rust#38006) with support for `T: Debug + ?Sized`, and `RwLock*Guard`s did allow `T: ?Sized` types already); the `Display` impls were added later (rust-lang/rust#42822) with support for `T: Debug + ?Sized` types. I think this needs a T-libs-api FCP? I'm not sure if this also needs an ACP. If so I can make one. These are changes to (stable) trait impls on stable types so will be insta-stable. `@rustbot` label +T-libs-api

rust-highfive assigned brson Jun 22, 2017

Mark-Simulacrum reviewed Jun 22, 2017

View reviewed changes

frewsxcv reviewed Jun 22, 2017

View reviewed changes

frewsxcv added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jun 22, 2017

alexcrichton reviewed Jun 22, 2017

View reviewed changes

alexcrichton added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 22, 2017

alexcrichton assigned alexcrichton and unassigned brson Jun 22, 2017

Mark-Simulacrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 22, 2017

Ensure Guard types impl Display & Debug

14df549

Fixes rust-lang#24372

ChrisMacNaughton force-pushed the guard-traits branch from c5e6810 to 14df549 Compare June 22, 2017 14:55

Mark-Simulacrum added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 22, 2017

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jun 22, 2017

Rollup merge of rust-lang#42822 - ChrisMacNaughton:guard-traits, r=al…

a419427

…excrichton Ensure Guard types impl Display & Debug Fixes rust-lang#24372

Mark-Simulacrum mentioned this pull request Jun 22, 2017

Rollup of 9 pull requests #42845

Closed

Mark-Simulacrum mentioned this pull request Jun 23, 2017

Rollup of 8 pull requests #42846

Closed

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jun 23, 2017

Rollup merge of rust-lang#42822 - ChrisMacNaughton:guard-traits, r=al…

bb2db94

…excrichton Ensure Guard types impl Display & Debug Fixes rust-lang#24372

Mark-Simulacrum mentioned this pull request Jun 23, 2017

Rollup of 8 pull requests #42856

Merged

bors added a commit that referenced this pull request Jun 23, 2017

Auto merge of #42856 - Mark-Simulacrum:rollup, r=Mark-Simulacrum

229d0d3

Rollup of 8 pull requests - Successful merges: #42777, #42783, #42787, #42821, #42822, #42825, #42829, #42833 - Failed merges:

bors merged commit 14df549 into rust-lang:master Jun 23, 2017

brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Jun 23, 2017

zachs18 mentioned this pull request Oct 24, 2023

Add T: ?Sized to RwLockReadGuard and RwLockWriteGuard's Debug impls. #117138

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure Guard types impl Display & Debug #42822

Ensure Guard types impl Display & Debug #42822

ChrisMacNaughton commented Jun 22, 2017 •

edited by sfackler

Loading

rust-highfive commented Jun 22, 2017

Mark-Simulacrum Jun 22, 2017

ChrisMacNaughton Jun 22, 2017

frewsxcv Jun 22, 2017

ChrisMacNaughton Jun 22, 2017

frewsxcv commented Jun 22, 2017

alexcrichton left a comment

alexcrichton Jun 22, 2017

ChrisMacNaughton Jun 22, 2017

ChrisMacNaughton Jun 22, 2017

alexcrichton Jun 22, 2017

alexcrichton Jun 22, 2017

ChrisMacNaughton Jun 22, 2017

alexcrichton Jun 22, 2017

Mark-Simulacrum commented Jun 22, 2017

ChrisMacNaughton commented Jun 22, 2017

Mark-Simulacrum commented Jun 22, 2017

ChrisMacNaughton commented Jun 22, 2017

sfackler commented Jun 22, 2017

ChrisMacNaughton commented Jun 22, 2017

alexcrichton commented Jun 22, 2017

bors commented Jun 22, 2017

Mark-Simulacrum commented Jun 22, 2017

Ensure Guard types impl Display & Debug #42822

Ensure Guard types impl Display & Debug #42822

Conversation

ChrisMacNaughton commented Jun 22, 2017 • edited by sfackler Loading

rust-highfive commented Jun 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frewsxcv commented Jun 22, 2017

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mark-Simulacrum commented Jun 22, 2017

ChrisMacNaughton commented Jun 22, 2017

Mark-Simulacrum commented Jun 22, 2017

ChrisMacNaughton commented Jun 22, 2017

sfackler commented Jun 22, 2017

ChrisMacNaughton commented Jun 22, 2017

alexcrichton commented Jun 22, 2017

bors commented Jun 22, 2017

Mark-Simulacrum commented Jun 22, 2017

ChrisMacNaughton commented Jun 22, 2017 •

edited by sfackler

Loading