Open eightfilms opened 4 months ago
@mimoo agreed on compiler warnings being useful to address possible critical bugs, tackling this PR has helped discover these problems. We can hold off on this PR for now and create issues to handle the more important warnings such as the unused results or variables, so that this PR ends up being just simple fixes that are just simple QoL changes.
I was thinking, if you remove anything that's not in the scope of this PR, and it contains only easy fixes to clippy, then we can merge it in : o
@mimoo makes sense, I'll remove the ones you mentioned. Seems like the easy fixes are:
panic!
to assert!
#[must_use]
annotations~I thought the must_use were false positives? they don't seem correct
I did a second pass through after removing all #[must_use]
annotations, seems like most of the changes now are trivial changes.
@mimoo Got caught up with other matters and left this PR in the dust. I will have time tomorrow to address your comments, sorry!
sry for reviving this ^^ just pointed out more places that I think we should not fix for now
There's a lot of changes - I'll briefly summarize what I did:
I ran, as much as possible, the following:
Some parts, I had to manually fix, or was out of scope of the current PR, so I added lint allowances + TODOs instead. An example of such 'out of scope' fixes include the
unused_must_use
s inr1cs/snarkjs.rs
- these would require additional work on error handling which should probably belong in its own PR. For these cases, I prefer lint allowances since they allow us to easily find the spots where we want to fix later on.Note that some of the lint allowances seem sane, so I didn't add TODOs for those, but only for those I felt would improve code quality if removed.