Implement Hash for schnorrsig::Signature
#335
Conversation
src/schnorrsig.rs
Outdated
| impl std::hash::Hash for Signature { | ||
| fn hash<H: std::hash::Hasher>(&self, state: &mut H) { | ||
| self.0.hash(state) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I recently learned that it is considered harmful to derive Hash/PartialEq and hand-implement the other and clippy complains about that (not sure if it would here, the PartialEq impl is behind a macro). The other question would be: why not derive most of the stuff currently in impl_array_newtype!?
There was a problem hiding this comment.
Because the stuff in impl_array_newtype needs to apply to array sizes for which derive does not work.
Why impl Hash here but not in impl_array_newtype?
There was a problem hiding this comment.
Why impl
Hashhere but not inimpl_array_newtype?
There already were some custom impls, updated the PR to remove them and instead implement Hash in the macro.
37b9d77 to
bac7890
Compare
|
|
* implements `Hash` as part of the newtype macro * removes type-specific implementations
bac7890 to
75b49ef
Compare
|
Right, didn't think of nostd 😅 |
|
Note: This was actually a breaking change because the macro is publicly exported. We are using it in https://github.com/ElementsProject/rust-secp256k1-zkp where it broken the build because we are adding those Not a big deal as it is easily fixed but something worth considering for the future. Should we export this macro? Should we not use it in https://github.com/ElementsProject/rust-secp256k1-zkp for types where we could derive things? It feels a bit like a "god"-macro to me that gets slapped on most newtypes without considering, what we actually need. Perhaps we should modularise this a bit more. |
|
Lol, oops. I think we shouldn't export it. |
I pondered putting the impl into the array type macro together with
(Partial)Eq, but that would have meant removing other implementations and potentially implementing it for types where it is not wanted. The drawback of the separate impl is that it is more disconnected from the(Partial)Eqimpl and could theoretically diverge (although unlikely in case of such a simple type) which would break the trait's contract.