-
Notifications
You must be signed in to change notification settings - Fork 27
Upgrade IPA to Rust 2024 edition #1532
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1532 +/- ##
==========================================
+ Coverage 91.81% 91.89% +0.07%
==========================================
Files 220 220
Lines 38229 38464 +235
==========================================
+ Hits 35099 35345 +246
+ Misses 3130 3119 -11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@andyleiserson @martinthomson it is ready for review now |
martinthomson
left a comment
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 all looks like rustfmt changes for the most part. Is there anything in particular I need to be looking at, or is this a case where I just trust Alex?
| timestamp, | ||
| epsilon, | ||
| sensitivity, | ||
| ] = s.splitn(8, ',').collect::<Vec<_>>()[..] |
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.
No sense in using SmallVec here, is there?
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.
probably no reason just yet as I don't think this one is on the hot path
| let b = rng.gen(); | ||
| let c = rng.gen(); | ||
| let d = rng.gen(); | ||
| let a = rng.r#gen(); |
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.
Is the rand crate getting updated? This is pretty unfortunate.
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.
yes, it is updated and the latest version (0.9 I believe) does not have this method anymore. Not every crate that IPA uses and depends on rand migrated over :(
I would say this is the case we should trust our CI to catch issues. You're right - most changes are just rustfmt |
For some reason formatting rules have changed it seems from 2021 to 2024 so there is a lot of noise here.
Other than that, there are only two major things
genis a reserved word now