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

Add impl_fmt_traits macro #90

Merged
merged 1 commit into from
May 20, 2024
Merged

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented May 16, 2024

Add a macro that implements the fmt::{LowerHex, UpperHex, Display, Debug} traits. Includes support for displaying backwards.

Context

In bitcoin_hashes we have two versions of this code, moving it here is better because it is general hex related code. Note however that the reason for supporting display backwards might see strange unusual to the casual reader.

Note

Note please the use of naked core::foo in the macro requiring core to be in scope, is this reasonable of should we do the ugly re-export stuff we do in bitcoin_hashes: $crate::_export::_core::foo?

@tcharding tcharding changed the title Add hex_fmt_impl macro Add impl_fmt_traits macro May 16, 2024
@tcharding
Copy link
Member Author

PR is tested in rust-bitcoin/rust-bitcoin#2770

@tcharding tcharding force-pushed the 05-16-fmt branch 2 times, most recently from 52d907b to a06b7ed Compare May 16, 2024 04:10
@tcharding tcharding changed the title Add impl_fmt_traits macro Add impl_fmt_traits! macro May 16, 2024
@tcharding tcharding changed the title Add impl_fmt_traits! macro Add impl_fmt_traits macro May 16, 2024
@apoelstra
Copy link
Member

In a06b7ed:

Concept ACK. But

  • We probably should do the ugly core stuff, since it guards against the case where a user has their own core module in scope.
  • We should change the interface to look more Rust-like, e.g.
impl_fmt_traits! {
    #[reverse(true)] // or whatever
    impl<T: Whatever> all_fmt_traits for MyType<T> {
        const LENGTH: usize = 32;
    }
}

@tcharding
Copy link
Member Author

tcharding commented May 16, 2024

woops, macro layout stuff sill to come. All review suggestions implented.

@tcharding tcharding force-pushed the 05-16-fmt branch 2 times, most recently from 4ba1693 to 7ddf1c0 Compare May 17, 2024 00:08
Add a macro that implements the `fmt::{LowerHex, UpperHex, Display,
Debug}` traits. Includes support for displaying backwards.
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK cb9b86a I am a tiny bit tempted to say that the const LENGTH should wind up as an actual associated constant. Though this is easy to add in a followup PR

@apoelstra apoelstra merged commit 7102dba into rust-bitcoin:master May 20, 2024
11 checks passed
@tcharding tcharding deleted the 05-16-fmt branch May 20, 2024 22:43
@Kixunil
Copy link
Collaborator

Kixunil commented Jun 30, 2024

@apoelstra that's impossible because you can't write [u8; Self::LENGTH * 2] and declaring the constant anyway is weird since none of the traits have it.

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