-
Notifications
You must be signed in to change notification settings - Fork 80
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
Don’t needlessly take ownership of argument in ChainId::new #721
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #721 +/- ##
==========================================
- Coverage 72.45% 72.24% -0.22%
==========================================
Files 117 117
Lines 15514 15557 +43
==========================================
- Hits 11241 11239 -2
- Misses 4273 4318 +45
☔ View full report in Codecov by Sentry. |
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.
Looks great! Can you add a changelog entry under .changelog/unreleased/breaking-changes/
?
This is API breaking change. ChainId::new allocates a new String so there’s no point in passing ownership of name argument. The constructor can accept it as a str slice without loss of functionality. This reduces allocations when creating ids since caller no longer has to have an owned string.
Done. |
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 is API breaking change. ChainId::new allocates a new String so there’s no point in passing ownership of name argument. The constructor can accept it as a str slice without loss of functionality. This reduces allocations when creating ids since caller no longer has to have an owned string.
This is API breaking change.
ChainId::new allocates a new String so there’s no point in passing ownership of name argument. The constructor can accept it as a str slice without loss of functionality. This reduces allocations when creating ids since caller no longer has to have an owned string.
PR author checklist:
unclog
.docs/
).Reviewer checklist:
Files changed
in the GitHub PR explorer.