Use hex from internals rather than hashes #1476
Conversation
Kixunil
added
code quality
Kixunil
force-pushed
the
hex-refactor
branch
from
783739e
to
6efd250
Compare
|
Blocked on #1477 |
There was a problem hiding this comment.
I noticed that perhaps the Hash trait should require fmt::UpperHex a well?
This review is for the hashes::internal_macros and hashes::util changes only, I didn't review deeply the new stuff in internals::hex yet, will wait til there is a clean build so I can play with it locally to fully understand the improvements.
| #[macro_export] | ||
| /// Adds hexadecimal formatting implementation of a trait `$imp` to a given type `$ty`. | ||
| macro_rules! hex_fmt_impl( |
| @@ -113,7 +121,7 @@ macro_rules! hash_newtype { | |||
| #[repr(transparent)] | |||
| pub struct $newtype($hash); | |||
|
|
|||
| $crate::hex_fmt_impl!($newtype); | |||
| $crate::hex_fmt_impl!($reverse, $newtype); | |||
There was a problem hiding this comment.
Can we have $reverse after $newtype so the args are ordered the same as for other macros.
There was a problem hiding this comment.
Generics have to be at the end and I think it'd be a bit weird to have $reverse between type name and generics. Agree this one is not great either. 🤷♂️
There was a problem hiding this comment.
What if we had a separate macro lpmi_tmf_xeh which was the reversed version, rather than having a parameter for $reverse.
ducks
| @@ -21,14 +68,32 @@ | |||
| /// `internal_engine` is required to initialize the engine for given hash type. | |||
| macro_rules! hash_trait_impls { | |||
| ($bits:expr, $reversed:expr $(, $gen:ident: $gent:ident)*) => { | |||
There was a problem hiding this comment.
Unrelated but at some stage it would be nice to use a single term, currently we have $reversed and $reverse - I think we should favour $reverse because it matches DISPLAY_BACKWARD (i.e., $reversed pairs with DISPLAYED_BACKWARD).
Kixunil
force-pushed
the
hex-refactor
branch
from
6efd250
to
f600dba
Compare
`bitcoin-internals` contains a more performant implementation of hex encoding than what `bitcoin_hashes` uses internally. This switches the implementations for formatting trait implementations as a step towards moving over completely. The public macros are also changed to delegate to inner type which is technically a breaking change but we will break the API anyway and the consuers should only call the macro on the actual hash newtypes where the inner types already have the appropriate implementations. Apart from removing reliance on internal hex from public API this reduces duplicated code generated and compiled. E.g. if you created 10 hash newtypes of SHA256 the formatting implementation would be instantiated 11 times despite being the same. To do all this some other changes were required to the hex infrastructure. Mainly modifying `put_bytes` to accept iterator (so that `iter().rev()` can be used) and adding a new `DisplayArray` type. The iterator idea was invented by Tobin C. Harding, this commit just adds a bound check and generalizes over `u8` and `&u8` returning iterators. While it may seem that `DisplayByteSlice` would suffice it'd create and initialize a large array even for small arrays wasting performance. Knowing the exact length `DisplayArray` fixes this. Another part of refactoring is changing from returning `impl Display` to return `impl LowerHex + UpperHex`. This makes selecting casing less annoying since the consumer no longer needs to import `Case` without cluttering the API with convenience methods.
Kixunil
force-pushed
the
hex-refactor
branch
from
f600dba
to
3e520f9
Compare
|
Legend, making hex progress now! |
bitcoin-internalscontains a more performant implementation of hex encoding than whatbitcoin_hashesuses internally. This switches the implementations for formatting trait implementations as a step towards moving over completely.The public macros are also changed to delegate to inner type which is technically a breaking change but we will break the API anyway and the consuers should only call the macro on the actual hash newtypes where the inner types already have the appropriate implementations.
Apart from removing reliance on internal hex from public API this reduces duplicated code generated and compiled. E.g. if you created 10 hash newtypes of SHA256 the formatting implementation would be instantiated 11 times despite being the same.
To do all this some other changes were required to the hex infrastructure. Mainly modifying
put_bytesto accept iterator (so thatiter().rev()can be used) and adding a newDisplayArraytype. The iterator idea was invented by Tobin C. Harding, this commit just adds a bound check and generalizes overu8and&u8returning iterators.While it may seem that
DisplayByteSlicewould suffice it'd create and initialize a large array even for small arrays wasting performance. Knowing the exact lengthDisplayArrayfixes this.Another part of refactoring is changing from returning
impl Displayto returnimpl LowerHex + UpperHex. This makes selecting casing less annoying since the consumer no longer needs to importCasewithout cluttering the API with convenience methods.