clippy: deny perf and redundant_clone lints#10121
Conversation
d69ed1d to
7bba583
Compare
|
The Firedancer team maintains a line-for-line reimplementation of the |
|
I believe this is a good change to have. @brooksprumo and @steviez would you mind having a look? @nagisa I think you need to run |
If you do not specify the same nightly version, then you may end up with different formatting and still fail CI. Here's the cmd you want: or what I do is run which uses the repo's |
brooksprumo
left a comment
There was a problem hiding this comment.
Signing off for these files:
- accounts-db/src/accounts.rs
- core/tests/snapshots.rs
- runtime/src/bank/accounts_lt_hash.rs
- runtime/src/serde_snapshot.rs
And also the snapshot changes in core/src/validator.rs.
There was a problem hiding this comment.
and figured that we might as well enable the perf related lints in our code to get less of these issues landed in the first place
Sounds great and agree with the direction. Same nit / question as Brooks; otherwise, I'll gradually review. Looks like you have a merge conflict in addition to the fmt issue that failed CI. I'll +1 Brooks' comment that I do the following to ensure I run the correct version
./cargo nightly fmt
1df7442 to
8f1e4d0
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10121 +/- ##
=========================================
- Coverage 82.9% 82.9% -0.1%
=========================================
Files 839 839
Lines 316033 316033
=========================================
- Hits 262233 262226 -7
- Misses 53800 53807 +7 🚀 New features to boost your workflow:
|
| fn test_access_violation_handler() { | ||
| let program_id = Pubkey::new_unique(); | ||
| let shared_account = AccountSharedData::new(0, 4, &program_id); | ||
| let shared_account_ref = shared_account.clone(); |
There was a problem hiding this comment.
On the surface, it isn't clear to me why creating shared_account_ref and then explicitly dropping later is required
There was a problem hiding this comment.
yeah, fair enough. I'll think about how to express this in a more self-explanatory way...
There was a problem hiding this comment.
I ended up keeping the original flow but added an explicit use of shared_account closer to the asserts to reinforce the shared nature of shared_account.
|
To move this from standstill, could we split this into smaller, more focused PRs which would not require multiple teams to approve? |
|
Sure. I can do that… later. |
These are detected by the `clippy::redundant_clone` lint which I hope to deny by default in near future. ref. anza-xyz#10121
These are detected by the `clippy::redundant_clone` lint which I hope to deny by default in near future. ref. anza-xyz#10121
These are detected by the `clippy::redundant_clone` lint which I hope to deny by default in near future. ref. anza-xyz#10121
These are detected by the `clippy::redundant_clone` lint which I hope to deny by default in near future. ref. anza-xyz#10121
These are detected by the `clippy::redundant_clone` lint which I hope to deny by default in near future. ref. anza-xyz#10121
These are detected by the `clippy::redundant_clone` lint which I hope to deny by default in near future. ref. #10121
These are detected by the `clippy::redundant_clone` lint which I hope to deny by default in near future. ref. anza-xyz#10121
These are detected by the `clippy::redundant_clone` lint which I hope to deny by default in near future. ref. anza-xyz/agave#10121
These are detected by the `clippy::redundant_clone` lint which I hope to deny by default in near future. ref. anza-xyz/agave#10121
These are detected by the `clippy::redundant_clone` lint which I hope to deny by default in near future. ref. anza-xyz/agave#10121
|
This for the most part has been whittled down to just core libraries and the vote program. I think this is now small enough that it should be fine to land with one approving reviewer. |
|
Double-checked just now that with master merged in the clippy passes so there should be no fallout. |
These are detected by the `clippy::redundant_clone` lint which I hope to deny by default in near future. ref. anza-xyz/agave#10121
These are detected by the `clippy::redundant_clone` lint which I hope to deny by default in near future. ref. anza-xyz/agave#10121
These are detected by the `clippy::redundant_clone` lint which I hope to deny by default in near future. ref. anza-xyz/agave#10121
These are detected by the `clippy::redundant_clone` lint which I hope to deny by default in near future. ref. anza-xyz/agave#10121
Problem / Summary of Changes
I saw #10119 and figured that we might as well enable the perf related lints in our code to get less of these issues landed in the first place. Interestingly, though, this lint did not detect the exact problem in
svmand I'm surprised there were no issues thatclippy::perfflagged, so I'd be willing to drop that part of the PR.