Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Don’t needlessly take ownership of argument in ChainId::new #721

Merged
merged 1 commit into from
Jun 21, 2023

Conversation

mina86
Copy link
Contributor

@mina86 mina86 commented Jun 17, 2023

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:

  • Added changelog entry, using unclog.
  • Added tests.
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).
  • Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

@codecov
Copy link

codecov bot commented Jun 20, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.22 ⚠️

Comparison is base (5d1855b) 72.45% compared to head (fbc966e) 72.24%.

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     
Impacted Files Coverage Δ
...ibc/src/core/ics02_client/handler/update_client.rs 95.12% <ø> (ø)
crates/ibc/src/mock/ics18_relayer/context.rs 67.44% <ø> (ø)
...s/ibc/src/clients/ics07_tendermint/client_state.rs 70.01% <100.00%> (ø)
...src/core/ics03_connection/handler/conn_open_ack.rs 96.62% <100.00%> (ø)
...src/core/ics03_connection/handler/conn_open_try.rs 94.08% <100.00%> (ø)
crates/ibc/src/core/ics24_host/identifier.rs 59.15% <100.00%> (-0.38%) ⬇️
crates/ibc/src/mock/context.rs 73.90% <100.00%> (ø)

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@plafer plafer left a 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.
@mina86
Copy link
Contributor Author

mina86 commented Jun 20, 2023

Done.

@mina86 mina86 requested a review from plafer June 20, 2023 21:37
Copy link
Contributor

@plafer plafer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏

@plafer plafer merged commit 62575ce into informalsystems:main Jun 21, 2023
@mina86 mina86 deleted the b branch June 21, 2023 13:59
@Farhad-Shabani Farhad-Shabani added the A: breaking Admin: breaking change that may impact operators label Jun 21, 2023
@Farhad-Shabani Farhad-Shabani added this to the v0.42.0 milestone Jun 27, 2023
Farhad-Shabani pushed a commit that referenced this pull request Sep 9, 2024
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.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A: breaking Admin: breaking change that may impact operators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants