Remove Index trait bound from Hash
#168
Conversation
tcharding
force-pushed
the
08-09-rm-index-impls
branch
2 times, most recently
from
99d38be to
5d23126
Compare
|
hmm, the CI fail appears to be on master right now. Will investigate more. |
|
Yeah, I can get behind this. When I added I do think that |
|
CI needs #167 to go green |
|
Ok, merged #167. Can you rebase this? |
tcharding
force-pushed
the
08-09-rm-index-impls
branch
from
5d23126 to
54ebf44
Compare
|
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. |
According to the Rust docs [0] on the `Deref` trait, it should only be implemented for smart pointers. Remove the implementation of `Deref`. [0] https://doc.rust-lang.org/std/ops/trait.Deref.html
The `Index` trait is not required if we provide the `AsRef` trait. Users can just call `as_ref()[0]` to get access to the 0th element of a `Hash`. For times when we take a reference to the whole slice `[..]` using `as_ref()` is capable and arguably more ergonomic. From the `Hash` trait remove the trait bound on `Index` and its associated traits, instead use the trait bound `AsRef` (we already have a trait bound of `Borrow`). Fix all uses of `foo[..]` -> `foo.as_ref()`.
tcharding
force-pushed
the
08-09-rm-index-impls
branch
from
54ebf44 to
b13b06a
Compare
|
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
|
|
I've added |
We now implement `AsRef` for hash types, sometimes usage of `as_ref` may required type annotations. For such cases we can provide an alternate way to get the underlying bytes to make the API more ergonomic. Add a method `as_bytes` for getting a reference to the inner byte array. Add for new types created by `hash_newtype` also.
We implement `as_bytes` on hash types; in order to be able to modify a hash it would be useful to iterate the inner byte array mutable. Add a `as_mut_bytes` method to all hash types and also to any hash created with `hash_newtype`.
tcharding
force-pushed
the
08-09-rm-index-impls
branch
from
357732b to
be8b9f1
Compare
|
I've added |
|
@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 rust-bitcoin/rust-bitcoin#1284 |
4 participants
The
Indextrait is not required if we provide theAsReftrait. Userscan just call
as_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
Hashtrait remove the trait bound onIndexand itsassociated traits, instead use the trait bound
AsRef(we already havea trait bound of
Borrow).The first patch removes
Derefimplementations, I did this becausethe 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 rust-bitcoin/rust-miniscript#450 (comment)
Verification
This PR is verified to not break usage of the lib by patching
rust-secp256k1andrust-bitcoin.