A few cleanup commits #200
Conversation
The original doccomment was a self-contained single-sentence description of this iterator. But it was really long and hard to follow. Better to separate out some details into a separate paragraph.
Substantially rewrite the `encode_decode` fuzztest, which had some ugly/awkward control flow. For the other tests, remove the AFL feature which has not been used in years and the now-unneeded `extern crate` and `macro_use` calls. Now you can fuzz things by just running `cargo hfuzz` directly without needing to set rustflags etc. Also run shellcheck on fuzz.sh, add a bunch of quotes to it, and change the runtime from 100000 iterations (which seems to run in less than a second on my system for all the existing tests) to a fixed 10 seconds. This does **not** pull the full rust-bitcoin fuzztest scripts over. I would like to wait until the "move fuzztests to cron" PR merges in rust-bitcoin, and consider trying to move all the scripts into a shared repo rather than copy/pasting them in, before doing so.
Also tighten the ops::Neg bound to specify that the output needs to be Self. Otherwise this bound is not really that useful.
Apparently v2 is deprecated and no longer supported. v3 works for us and v4 has some new limit on the size of uploads, but those shouldn't be a problem here, so jump all the way to v4.
| fn sum<I: Iterator<Item = &'s Self>>(iter: I) -> Self { | ||
| iter.fold(crate::primitives::Field::ZERO, |i, acc| i + acc) | ||
| } | ||
| } |
There was a problem hiding this comment.
Is there any technical reason you chose to not add a _sum trait method to the Field trait and call it here like the other code does? If not I rekon we should keep it uniform.
There was a problem hiding this comment.
The reason the _add etc methods exist is so that I could implement the 4 different Add traits with a single macro call.
There is only one Sum trait to implement and it can be implemented in terms of _add. I don't see what the value of having a _sum method would be. Would I insist that implementors implement it?
There was a problem hiding this comment.
If anything I should delete the _add methods and improve the macro. I may do this in a followup.
There was a problem hiding this comment.
I read the code a bit more, I thought the _foo methods were just to make impl_ops_for_fe less duplicated, my mistake.
|
Everything else looks good, I even checked |
|
@clarkmoody to get the fuzzer working locally you may need to install a different version of |
This extracts 3 (hopefully) simple and non-controversial commits from my error-correction branch.
The first is just a clippy fix.
The second cleans up and simplifies the fuzztests (it does not bring over the new fuzztesting scripts from rust-bitcoin, use the bitcoin-maintainer-tools repo, introduce libfuzzer, etc.; it just does some basic cleanups to make things more useful).
And the third adds a few new bounds to the
Fieldtrait, specificallyNeg<Output = Self>andSum. (This is a breaking change.)