Skip to content
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

Removing saturating_add_assign and replacing with Saturating struct #4029

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

roryharr
Copy link

Problem

Cleaning up all usages of saturating add assign and replacing with Saturating
#502

Summary of Changes

Fixes #

@roryharr roryharr requested a review from a team as a code owner December 10, 2024 01:14
@steviez steviez added the CI Pull Request is ready to enter CI label Dec 10, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Dec 10, 2024
steviez
steviez previously approved these changes Dec 10, 2024
Copy link

@steviez steviez left a comment

Choose a reason for hiding this comment

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

Thank, LGTM! I applied the CI label but we'll also need to let someone from svm team take a look too

LucasSte
LucasSte previously approved these changes Dec 10, 2024
@LucasSte
Copy link

The CI has been failing in a check: https://buildkite.com/anza/agave/builds/15596#0193af2b-68f5-4b10-84d9-f316e42f9886

Would you mind fixing that?

@roryharr roryharr dismissed stale reviews from LucasSte and steviez via 11a28f8 December 10, 2024 16:56
@mergify mergify bot requested a review from a team December 10, 2024 16:57
@roryharr
Copy link
Author

The CI has been failing in a check: https://buildkite.com/anza/agave/builds/15596#0193af2b-68f5-4b10-84d9-f316e42f9886

Would you mind fixing that?

Resolved

@LucasSte LucasSte added the CI Pull Request is ready to enter CI label Dec 10, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Dec 10, 2024
@roryharr
Copy link
Author

Clippy failure as well. Will grab that, and run locally from now on.

@steviez steviez added the CI Pull Request is ready to enter CI label Dec 10, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Dec 10, 2024
@steviez
Copy link

steviez commented Dec 10, 2024

In the future, the following are good command to run from the base of the repo:

$ ./cargo nightly fmt
$ ./cargo nightly clippy
$ cargo test --no-run

The first two will keep the linters happy, the last one will ensure all test code can be compiled oto

@steviez steviez added the CI Pull Request is ready to enter CI label Dec 10, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Dec 10, 2024
@roryharr
Copy link
Author

roryharr commented Dec 10, 2024

Clippy nightly has a bunch of errors right now that seem unrelated. Is there a set of rules that should be ignored?
For example:

warning: the following explicit lifetimes could be elided: 'a
--> core/src/repair/repair_generic_traversal.rs:25:6
|
25 | impl<'a> Iterator for GenericTraversal<'a> {
| ^^ ^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_lifetimes
help: elide the lifetimes
|
25 - impl<'a> Iterator for GenericTraversal<'a> {
25 + impl Iterator for GenericTraversal<'_> {

@steviez
Copy link

steviez commented Dec 10, 2024

Clippy nightly has a bunch of errors right now that seem unrelated. Is there a set of rules that should be ignored? For example: warning: the following explicit lifetimes could be elided: 'a --> core/src/repair/repair_generic_traversal.rs:25:6 | 25 | impl<'a> Iterator for GenericTraversal<'a> { | ^^ ^^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_lifetimes help: elide the lifetimes | 25 - impl<'a> Iterator for GenericTraversal<'a> { 25 + impl Iterator for GenericTraversal<'_> {

Are you sure you're using the correct nightly version ? If you use the one specified by ./cargo nightly, then that should build with what we build with. For the specific failure you hit this time, you need to rebase to tip of master to pick up #4023:

error: this function has too many arguments (10/9)
--
  | --> accounts-cluster-bench/src/main.rs:749:1
  | \|
  | 749 \| / fn make_rpc_bench_threads(
  | 750 \| \|     rpc_benches: Vec<RpcBench>,
  | 751 \| \|     mint: &Option<Pubkey>,
  | 752 \| \|     start_bench_barrier: &Arc<Barrier>,
  | ...   \|
  | 759 \| \|     transaction_signature_tracker: &TransactionSignatureTracker,
  | 760 \| \| ) -> Vec<JoinHandle<()>> {

@roryharr
Copy link
Author

Thanks, I see the issue. I was using cargo +nightly, rather than ./cargo nightly

@roryharr roryharr force-pushed the remove_saturating_add_svm_counters branch from e8672e5 to 195e81f Compare December 11, 2024 00:11
@roryharr roryharr force-pushed the remove_saturating_add_svm_counters branch from 195e81f to d5bba54 Compare December 11, 2024 00:13
@steviez
Copy link

steviez commented Dec 11, 2024

As another note - when you make changes to the PR, please push them as new commits instead of squashing them down.

  • If you push new commits, it is easier to see the couple lines that you have changed
  • If you squash everything together, we have to re-review the entire changeset

@steviez steviez added the CI Pull Request is ready to enter CI label Dec 11, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants