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

Implement `Debug` for public types #442

Merged
merged 8 commits into from Sep 22, 2017

Conversation

Projects
None yet
4 participants
@tmccombs
Copy link
Contributor

tmccombs commented Sep 18, 2017

Fixes #427.

I included what I though was important in the implementations, but I'm open to suggestions.

@nikomatsakis

This comment has been minimized.

Copy link
Member

nikomatsakis commented Sep 18, 2017

@tmccombs hmm -- I'm seeing a bunch of travis failures! do you mind taking a look?

@KodrAus

This comment has been minimized.

Copy link

KodrAus commented Sep 18, 2017

That'll be the deny(missing_debug), which includes pub structs even if they aren't re-exported.

@nikomatsakis would you rather keep the lint and work around pub(not pub) structs or just remove it?

@tmccombs

This comment has been minimized.

Copy link
Contributor Author

tmccombs commented Sep 18, 2017

Ah I didn't run with rayon_unstable feature turned on locally. I'll look into them.

@tmccombs tmccombs force-pushed the tmccombs:debug-impl branch from 6dede2b to 57d7725 Sep 19, 2017

@tmccombs

This comment has been minimized.

Copy link
Contributor Author

tmccombs commented Sep 19, 2017

hmm, I had a typo, so it didn't actually get everything, I'll work on getting all the structs I missed.

tmccombs added some commits Sep 19, 2017

@KodrAus
Copy link

KodrAus left a comment

Thanks @tmccombs! There's been a lot of structs to add Debug impls for.

This looks good to me.

@@ -113,6 +115,11 @@ impl<T, E> Drop for RayonFuture<T, E> {
}
}

impl<T, E> fmt::Debug for RayonFuture<T, E> {
fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
fmt.write_str("RayonFuture(...)")

This comment has been minimized.

@KodrAus

KodrAus Sep 21, 2017

This could probably just be fmt.debug_struct("RayonFuture").finish().

@KodrAus
Copy link

KodrAus left a comment

I'm happy with this, so I'll hand over to @nikomatsakis or @cuviper who might want to tweak the fields included for some of the Debug implementations.

fmt.debug_struct("Scope")
.field("owner_thread", &self.owner_thread)
.field("panic", &self.panic)
.field("job_completed", &self.job_completed_latch.probe())

This comment has been minimized.

@cuviper

cuviper Sep 22, 2017

Member

I think probe() will always be true until the scope is out of the user's hands. For debugging purposes, the actual counter value would be more interesting. How about adding Debug to CountLatch and show that here?

This comment has been minimized.

@tmccombs

tmccombs Sep 22, 2017

Author Contributor

alright

@cuviper

This comment has been minimized.

Copy link
Member

cuviper commented Sep 22, 2017

I added a couple more that hit master in the meantime.

Thanks!

@cuviper cuviper merged commit 29c6aad into rayon-rs:master Sep 22, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tmccombs tmccombs deleted the tmccombs:debug-impl branch Sep 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.