Closed tcharding closed 1 year ago
hmm, the CI fail appears to be on master right now. Will investigate more.
Yeah, I can get behind this. When I added Index
I think it [..]
was the fashionale way to convert slice-like objects to slices. Or maybe I just saw it done this way once and copied it.
I do think that .as_ref
is more explicit about what's going on, and a bit less less likely to happen by accident/confusingly.
CI needs https://github.com/rust-bitcoin/bitcoin_hashes/pull/167 to go green
Ok, merged #167. Can you rebase this?
Rebased!
I meant removing bound not impl. But since indexing into hash is kinda weird, I'm not against removing impl. Could be done in two steps.
cc @TheBlueMatt are you opposed to this? It's a reduction in the API surface which arguably needlessly breaks people (in a mechanically fixable way, but still annoying). But OTOH it makes the separation between hashes and byteslices a little cleaner.
ACK removing the bound.
Rebase only, no other changes.
I've ACKed but I would still like a (concept) ACK from @TheBlueMatt since we're removing API functions that are fairly widely used.
For consideration: some of these methods to avoid inference issues and possibly get better clarity than with as_ref
:
as_bytes(&self) -> &[u8; LEN]
as_byte_array(&self) -> &[u8; LEN]
as_bytes(&self) -> &[u8]
as_byte_slice(&self) -> &[u8]
as_mut_*
I've added as_bytes(&self) -> &[u8; LEN]
for all hash types as well as for types created by hash_newtype!
.
I've added as_mut_bytes
as well, as a separate patch. I'm not understanding the utility of this method though - why would one want to modify a hash making the preimage unknown? If we want a hash with an unknown pre-image we can just use Hash::all_zeros
.
@tcharding the only utility I can think of is consistency and I think three more lines are fine just for consistency.
Cool, cheers.
Closing in preparation for archiving this repo. See https://github.com/rust-bitcoin/rust-bitcoin/pull/1284
The
Index
trait is not required if we provide theAsRef
trait. Users can just callas_ref()[0]
to get access to the 0th element of aHash
.For times when we take a reference to the whole slice
[..]
usingas_ref()
is capable and arguably more ergonomic.From the
Hash
trait remove the trait bound onIndex
and its associated traits, instead use the trait boundAsRef
(we already have a trait bound ofBorrow
).The first patch removes
Deref
implementations, I did this because the docs say so - not sure if that is enough justification or if I'm missing something.https://doc.rust-lang.org/std/ops/trait.Deref.html
This work was inspired by https://github.com/rust-bitcoin/rust-miniscript/pull/450#discussion_r932704196
Verification
This PR is verified to not break usage of the lib by patching
rust-secp256k1
andrust-bitcoin
.