-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: added clippy to CI #74
Conversation
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.
Thank you for adding this!
It might be better to put this in its own workflow, so we can turn it off independently. What do you think?
Each workflow can be disabled here, but individual jobs or steps can’t:
https://github.com/worldcoin/gpu-iris-mpc/actions
If we don’t make that change, let’s create a separate job, so we have separate format and clippy status indicators?
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.
I’ve reviewed the other code changes and they look good. But let’s keep the safety assert.
src/bin/server.rs
Outdated
@@ -993,10 +986,12 @@ async fn main() -> eyre::Result<()> { | |||
// before running that code. | |||
// - End events are re-used in each thread, but we only end one thread at a | |||
// time. | |||
assert!(MAX_BATCHES_BEFORE_REUSE > MAX_CONCURRENT_BATCHES); | |||
// (Dragos) Commented assert here since it's going to be optimized by the | |||
// compiler assert!(MAX_BATCHES_BEFORE_REUSE > MAX_CONCURRENT_BATCHES); |
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.
This assertion is required for memory safety reasons.
To make sure it is checked at compile time, and silence the warning, replace it with static_assertions::const_assert!()
:
https://docs.rs/static_assertions/latest/static_assertions/macro.const_assert.html
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.
Fixed in 4054e6e
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.
I made both changes, and also fixed a YAML parser lint
Adding clippy check to CI