Skip to content

Switch back to automatic Debug derive for Header struct #435

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

Merged
merged 1 commit into from
May 29, 2022

Conversation

nicholasbishop
Copy link
Member

The Header Debug implementation was accidentally printing the size
field for the signature. Rather than just fix that field, switch the
whole thing back to using derive(Debug). This adds in the _reserved
field, which probably doesn't matter much either way but might as well
show the whole struct when debug printing.

@GabrielMajeri
Copy link
Collaborator

I've also discovered the Derivative crate which would allow us to use a custom Debug trait derivation with support for skipping certain fields. Would it be worth adding it as a dependency and using it?

The `Header` `Debug` implementation was accidentally printing the `size`
field for the `signature`. Rather than just fix that field, switch the
whole thing back to using `derive(Debug)`. This adds in the `_reserved`
field, which probably doesn't matter much either way but might as well
show the whole struct when debug printing.
@nicholasbishop
Copy link
Member Author

I've also discovered the Derivative crate which would allow us to use a custom Debug trait derivation with support for skipping certain fields. Would it be worth adding it as a dependency and using it?

I think in this case I would mildly prefer to continue printing the reserved field, because the spec mentions that it must be set to zero. So I see this as a bit different from a padding field, which could be safely set to any value. If you debug print the header and see the reserved field is not all zeros that could be useful info.

@GabrielMajeri
Copy link
Collaborator

Got it. LGTM then!

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.

2 participants