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
TaprootMerkleBranch
improvements
#2263
TaprootMerkleBranch
improvements
#2263
Conversation
d3665af
to
20a65c7
Compare
While `clippy` now allows `TBD` to be used in `since` parameter of `deprecated` attribute it is only available in the newest, nightly, version. Switch `clippy` version to nightly to enable the `TBD` value.
These names are more descriptive.
20a65c7
to
83f00d9
Compare
self.0.iter() | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have DerefMut
is there a reason we don't have IntoIterator for &'a mut TaprootMerkleBranch
?
I see we do get iter_mut
(from having Deref
to a slice I believe).
(I'm trying to deepen my understanding of all all these stdlib traits.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I forgot about that one!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 83f00d9
My comments are questions for my personal education, if you have the time and inclination to answer them please.
The type is naturally a collection of hashes so make it behave that way by implementing `Deref`, `AsRef`, `Borrow` and their mutable versions as well as `IntoIterator` for its reference. `IntoIterator` for itself is not yet implemented because it's a bit more complicated.
Since the iterator created by `IntoIterator` should be called `IntoIter` we move the whole `TaprootMerkleBranch` to its own module which contains the type to avoid confusion. This has an additional benefit of reducing the scope where the invariant could be broken. This already uncovered that our internal code was abusing access to the private field (although the code was correct). To implement the iterator we simply delegate to `vec::IntoIter`, including overriding the default method which are likely to be implemented by `Vec` more optimally. We avoid exposing `vec::IntoIter` directly since we may want to change the representation (e.g. to `ArrayVec`).
These functions either delegate to functions with identical signature or contain condition that may be optimized-out after inlining.
83f00d9
to
e1cc989
Compare
Added the missing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK e1cc989
/// | ||
/// # Errors | ||
/// If inner proof length is more than [`TAPROOT_CONTROL_MAX_NODE_COUNT`] (128). | ||
#[inline] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the benefit of inlining a private function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not thinking when I add attributes. :D But it's harmless so I suggest let's keep it unless anyone objects or the PR has to change anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good with me.
LGTM! Note that we're almost guaranteed to get breakages from using a nightly clippy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK e1cc989
If we want to use |
This contains several improvements to
TaprootMerkleBranch
that make the API more idiomatic.