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

Combine remove and add in update_accumulator_refresh_cache(). #5197

Closed
wants to merge 1 commit into from

Conversation

mstembera
Copy link
Contributor

@mstembera mstembera commented Apr 28, 2024

STC https://tests.stockfishchess.org/tests/view/662d96dc6115ff6764c7f4ca
LLR: 2.95 (-2.94,2.94) <0.00,2.00>
Total: 364032 W: 94421 L: 93624 D: 175987
Ptnml(0-2): 1261, 41983, 94811, 42620, 1341

Combine remove and add in update_accumulator_refresh_cache().
Move remove before add to match other parts of the code.
This includes and was tested on top of PR5194.
No functional change
bench: 1836777

Please let me know if I should close #5194 or something else to make it easier to commit on your end.

Put remove before add processing as in other parts of the code.
This is and was tested on top of PR5194.
No functional change
bench: 1836777
Copy link

clang-format 17 needs to be run on this PR.
If you do not have clang-format installed, the maintainer will run it when merging.
For the exact version please see https://packages.ubuntu.com/mantic/clang-format-17.

(execution 8866163513 / attempt 1)

@cj5716
Copy link
Contributor

cj5716 commented Apr 28, 2024

I wonder if we can do something similar in update_accumulator_incremental

@mstembera
Copy link
Contributor Author

@cj5716
Copy link
Contributor

cj5716 commented Apr 28, 2024

I see, thanks

@Disservin
Copy link
Member

Disservin commented Apr 28, 2024

IMO the ideal thing would be two have 1 PR with 2 commits.
First commit would be the content of the other PR and the second commit has the combine remove/add part.
Can you do that @mstembera ?

Which then can be merged without squashing.

@mstembera
Copy link
Contributor Author

@Disservin Ok I committed this on top of #5194 w/o squashing. Let me know if you need anything else.

@Disservin
Copy link
Member

Thanks. Then I'll close this one since your other pr will be merged then.

@Disservin Disservin closed this Apr 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants