-
Notifications
You must be signed in to change notification settings - Fork 679
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
Replace Vec References with slices in function inputs #3535
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #3535 +/- ##
============================================
- Coverage 65.55% 31.43% -34.13%
============================================
Files 240 298 +58
Lines 130905 275038 +144133
============================================
+ Hits 85819 86450 +631
- Misses 45086 188588 +143502
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
pub fn new(path: &[u8]) -> TrieNode4 { | ||
TrieNode4 { | ||
path: path.clone(), | ||
path: path.to_owned(), |
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.
Generally speaking in scenarios like this one, I'd argue it's better to take the owned type as input instead of cloning a reference. So I'd change the input to path: Vec<u8>
, leaving it up to the caller to decide when to clone.
I recognize that this is beyond the scope of the current refactor, so feel free to leave that for future improvements.
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.
Yeah, don't do this.
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.
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, thank you!
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.
Overall this looks good to me. You also need to update the announce_block
implementation on line 454 in testnet/stacks-node/src/event_dispatcher.rs:454:19
to make testnet/stacks-node
compile.
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, thanks for the PR!
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.
Thanks @asimm241!
Need unit tests to pass before we can merge @asimm241 |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Description
This PR replaces all vector references with slices. It uses Clippy's ptr_arg lint for reference.
Applicable issues