Clean up bip158 module #1180
Clean up bip158 module #1180
Conversation
The function `match_all` takes an iterator, it does not need to use the `Seek` trait, we can just pass in the content as a slice, no need to wrap it in a `Cursor`.
Currently in the `bip158` module we do a bunch of dynamic dispatch using the reader/writer objects. We can improve runtime performance, at the cost of compile time, by using generics instead of dynamic dispatch.
Conventionally we only put a single newline between types, impl blocks, and functions.
To ease reading, put the `new` method at the top of the impl block. Improve rustdocs while we do it.
Improve the module rustdocs for `bip158` by doing: - Add bip references - Use correct markdown headings - Use caps in title
Refactor the `new_script_filter` by doing: - Put it directly below the `new` constructor - Improve docs - Remove unneeded scope - Correctly indent `where` keyword
Rust convention stipulates that acronyms are in snake case not all caps. We should use `Gcs` for Golomb Coded Sets
Refactor the import statements for the bip158 tests module. Includes removing `extern crate` which is unnecessary now we have edition 2018.
Make an effort to improve the rustdocs throughout the `bip158` module.
7673377 to
167ee8e
Compare
|
Rebase to pick up |
| pub fn match_any(&self, block_hash: &BlockHash, query: &mut dyn Iterator<Item=&[u8]>) -> Result<bool, Error> { | ||
| pub fn match_any<'a, I>(&self, block_hash: &BlockHash, query: I) -> Result<bool, Error> | ||
| where | ||
| I: Iterator<Item = &'a [u8]>, |
There was a problem hiding this comment.
Could be further improved to accept I: Iterator, I::Item: Borrow<[u8]>
There was a problem hiding this comment.
I like this -- I wish I'd kept the PR open a bit longer. (Sorry to merge out from under you but it was a set of unoffensive cleanups and I wasn't sure how long you'd be busy.)
There was a problem hiding this comment.
No problem, can always be added later.
There was a problem hiding this comment.
Why Borrow instead of AsRef @Kixunil?
If generic code merely needs to work for all types that can provide a reference to related type T, it is often better to use AsRef as more types can safely implement it.
ref: https://doc.rust-lang.org/std/borrow/trait.Borrow.html
I tried both and they both work, since AsRef is more general I'll push a PR with that, waiting on your correction though if needed. Thanks
There was a problem hiding this comment.
It shouldn't really matter for this one. Lately I default to Borrow due to correctness implications, so I automatically wrote that one.
Note that there is non-correctness case when Borrow works and AsRef doesn't: when you have T: Sized and want to be able to take owned T - usually in structs but also Iter::Item: Borrow<T>, T: Copy.
In attempt to resolve #1147 clean up the
bip158module.I was unable to resolve the
"confusing method names read and write that look like those from std:i:o::{trait}"I find the
bip158data types and abstractions kind a funky but was unable to come up with any improvements.Open question: Are all the public data types really meant to be public? Perhaps we should have an issue to write an example module that uses the bip158 module?