-
-
Notifications
You must be signed in to change notification settings - Fork 384
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
Rewrite extensions in Rust #721
Conversation
CodSpeed Performance ReportMerging #721 will degrade performances by 18.61%Comparing Summary
Benchmarks breakdown
|
929c2bb
to
11d3b30
Compare
30835dc
to
a44aa76
Compare
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.
Some issues here and there, but overall, nice job. I am no Rust expert, so I just looked through it trusting that the test suite checks if it works correctly.
3ee1821
to
e35518e
Compare
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.
Great initiative, seeing C rewritten to Rust always makes me happy. I suggested some minor refactorings (partially suggested by clippy::pedantic
) but otherwise very good job. There are still few noticeable remnants of C that don't really fit into Rust (eg. the null char) but they require more complex changes and it isn't really necessary to do that.
Also, I am quite new to reviewing (got here fairly randomly) so sorry if there's anything wrong in that regard.
Thanks for the thorough review @Maneren! I applied all of your suggestions. |
Co-Authored-By: Maneren <[email protected]>
d6cf2fd
to
c197c0a
Compare
Happy to help |
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.
Few extra nitpicks
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.
LGTM, great job!
This PR is the first introduction to Rust in the codebase.
It is a complete rewrite of existing C extensions and paves the way to more speed improvements by rewriting other parts of the codebase in the future.
Note that if there is still a pure Python version of these extensions and will always be for the foreseeable future.
Be aware that this PR introduces small backwards incompatibilities when parsing ISO8601 strings:
T
, for instanceT201205
Pull Request Check List