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

tree-wide: Add Debug impls #53

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

cgwalters
Copy link
Collaborator

I find this really useful as a general baseline; I'm a "printf debugger" person by default. Add derives tree wide (plus two manual impls). Enable deny-by-default lint.

I find this really useful as a general baseline; I'm a
"printf debugger" person by default. Add derives tree wide (plus two
manual impls). Enable deny-by-default lint.

Signed-off-by: Colin Walters <[email protected]>
@shawn111
Copy link
Contributor

shawn111 commented Dec 8, 2024

Do you consider add tokio tracing?

Copy link
Collaborator

@allisonkarlitskaya allisonkarlitskaya left a comment

Choose a reason for hiding this comment

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

I generally like the idea. Is this to make it easier to add quick debugging statements here and there?

I've been looking at making more use of the log crate as well (like in my erofs branch).

Is there a way to get this only enabled on debug builds? I've read that the code that this generates is a bit expensive, but it would be kinda nice to find out if any {:?} slips into production code. I understand that's supposed to be an anti-pattern...

@@ -1,3 +1,5 @@
#![deny(missing_debug_implementations)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hardcore.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there a way to get this only enabled on debug builds?

Not that I know of.

I've read that the code that this generates is a bit expensive,

Expensive...for compilation speed? Runtime? I think it used to in some cases generate a lot of code which would slow down compilation a bit, but I think it's really well optimized nowadays (it's builtin to the compiler).

https://doc.rust-lang.org/nightly/unstable-book/compiler-flags/fmt-debug.html exists (linked from https://github.com/johnthagen/min-sized-rust ) but I think you pretty much have to be writing #[no_std] to care is my impression, and we are quite far away from that.

but it would be kinda nice to find out if any {:?} slips into production code.

I have never had an issue with that, you have to pretty explicitly type {:?}, it doesn't exactly flow off the fingertips 😄

@allisonkarlitskaya
Copy link
Collaborator

Do you consider add tokio tracing?

I consider the oci pull code (which is the only thing we use tokio for) to be little more than a toy at the moment. We're actively discussing how we merge this together with the much more mature code in bootc.

@allisonkarlitskaya allisonkarlitskaya merged commit 873b82d into containers:main Dec 9, 2024
4 checks passed
@@ -63,6 +65,17 @@ pub struct SplitStreamWriter<'a> {
pub sha256: Option<(Sha256, Sha256HashValue)>,
}

impl std::fmt::Debug for SplitStreamWriter<'_> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the extra effort here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants