Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor to use debug_struct in several Debug impls #44775

Merged
merged 1 commit into from
Oct 10, 2017

Conversation

MaloJaffre
Copy link
Contributor

@MaloJaffre MaloJaffre commented Sep 22, 2017

Also use pad and derive Debug for Edge.

Fixes #44771.

@rust-highfive
Copy link
Collaborator

r? @sfackler

(rust_highfive has picked a reviewer for you, use r? to override)

@@ -919,7 +919,7 @@ impl<T> Drop for Sender<T> {
#[stable(feature = "mpsc_debug", since = "1.8.0")]
impl<T> fmt::Debug for Sender<T> {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "Sender {{ .. }}")
f.pad("Sender {{ .. }}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The {{ and }} need to be changed to { and } as this string is no longer being used as a format string.

Copy link
Contributor Author

@MaloJaffre MaloJaffre Sep 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks for the review!
I removed the double braces.

@estebank estebank added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 23, 2017
@sfackler
Copy link
Member

I don't think this really fixes what #44771 was talking about - the output is still going to be on a single line if you format with {:#?}.

@MaloJaffre
Copy link
Contributor Author

MaloJaffre commented Sep 25, 2017

I've tested on my machine that:

println!("{:#?}", std::sync::Mutex::new(5));

prints (because of debug_struct use in Debug impl):

Mutex {
    data: 5
}

But you're right that for e.g. mpsc::Sender, the output is only on one line, because I didn't use debug_struct (I must only output .., without a field name).

I think this is another related issue that we can fix later, because I just refactored this part to be more consistent with other Debug impls.

@carols10cents
Copy link
Member

ping @sfackler - do you think this can be merged as-is to fix part of the problem?

@sfackler
Copy link
Member

sfackler commented Oct 3, 2017

The proper change seems like fmt.debug_struct("Sender").finish() rather than fmt.pad("Sender { .. }").

@shepmaster shepmaster added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 6, 2017
@MaloJaffre
Copy link
Contributor Author

Using fmt.debug_struct("Sender").finish() will show Sender instead of Sender { .. }, is that an acceptable change of output?

@sfackler
Copy link
Member

sfackler commented Oct 8, 2017

Yep, debug representations aren't stable.

@MaloJaffre
Copy link
Contributor Author

I've rebased and applied the requested changes.

Mutex now uses a LockedPlaceholder struct with a custom Debug impl, like RefCell.

With these changes,

fn main() {
  let a = std::sync::Mutex::new(5);
  let b = a.lock().unwrap();
  println!("{:#?}", a);
}

prints:

Mutex {
    data: <locked>
}

@sfackler
Copy link
Member

sfackler commented Oct 8, 2017

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 8, 2017

📌 Commit 7b02a2d has been approved by sfackler

@carols10cents carols10cents 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-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 9, 2017
@kennytm
Copy link
Member

kennytm commented Oct 9, 2017

@bors r-

CI failed.

[01:14:12] thread '<unnamed>' panicked at 'assertion failed: `(left == right)`
[01:14:12]   left: `"Handle"`,
[01:14:12]  right: `"Handle { .. }"`', /checkout/src/libstd/sync/mpsc/select.rs:789:8
[01:14:12] thread '<unnamed>' panicked at 'assertion failed: `(left == right)`
[01:14:12]   left: `"Select"`,
[01:14:12]  right: `"Select { .. }"`', /checkout/src/libstd/sync/mpsc/select.rs:781:8
[01:14:13] thread '<unnamed>' panicked at 'assertion failed: `(left == right)`
[01:14:13]   left: `"Receiver"`,
[01:14:13]  right: `"Receiver { .. }"`', /checkout/src/libstd/sync/mpsc/mod.rs:3022:8
[01:14:13] thread '<unnamed>' panicked at 'assertion failed: `(left == right)`
[01:14:13]   left: `"Sender"`,
[01:14:13]  right: `"Sender { .. }"`', /checkout/src/libstd/sync/mpsc/mod.rs:3016:8
[01:14:13] thread '<unnamed>' panicked at 'assertion failed: `(left == right)`
[01:14:13]   left: `"SyncSender"`,
[01:14:13]  right: `"SyncSender { .. }"`', /checkout/src/libstd/sync/mpsc/mod.rs:3028:8
...
[01:14:23] failures:
[01:14:23]     sync::mpsc::select::tests::fmt_debug_handle
[01:14:23]     sync::mpsc::select::tests::fmt_debug_select
[01:14:23]     sync::mpsc::sync_tests::fmt_debug_recv
[01:14:23]     sync::mpsc::sync_tests::fmt_debug_sender
[01:14:23]     sync::mpsc::sync_tests::fmt_debug_sync_sender
[01:14:23] 
[01:14:23] test result: FAILED. 803 passed; 5 failed; 0 ignored; 0 measured; 0 filtered out

@MaloJaffre
Copy link
Contributor Author

Sorry for the failed CI, this should be fixed now.

@sfackler
Copy link
Member

sfackler commented Oct 9, 2017

Can we just get rid of those tests? It doesn't seem like they're doing anything useful.

@MaloJaffre
Copy link
Contributor Author

Sure, I've removed these tests and squashed commits.

@sfackler
Copy link
Member

sfackler commented Oct 9, 2017

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 9, 2017

📌 Commit 679457a has been approved by sfackler

kennytm added a commit to kennytm/rust that referenced this pull request Oct 10, 2017
Refactor to use `debug_struct` in several Debug impls

Also use `pad` and derive `Debug` for `Edge`.

Fixes rust-lang#44771.
bors added a commit that referenced this pull request Oct 10, 2017
Rollup of 9 pull requests

- Successful merges: #44775, #45089, #45095, #45099, #45101, #45108, #45116, #45135, #45146
- Failed merges:
@bors bors merged commit 679457a into rust-lang:master Oct 10, 2017
@MaloJaffre MaloJaffre deleted the debug-struct branch October 11, 2017 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants