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

Micro-optimization for SEE: remove a superfluous condition #5839

Closed
wants to merge 1 commit into from

Conversation

pb00068
Copy link
Contributor

@pb00068 pb00068 commented Jan 31, 2025

The removed condition can never be true, it's superfluous.
It never triggers even with a bench 16 1 20 run.
To met the condition it would imply that the previous recapture was done by a higher rated piece than a Queen.
This is only the case when the King recaptures and that's already handled in line 1161: (return (attackers & ~pieces(stm)) ? res ^ 1).
To maintain the code robust and understandable I added an assert and a comment.
Thanks to mstembera in helping me for this PR.

no functional change
bench: 1767398

It never triggers even with a bench 16 1 20 run.
To met the condition it would imply that the previous recapture was done
by a higher rated piece than a Queen.
This is only the case when the King recaptures and that's already
handled in line 1161: (return (attackers & ~pieces(stm)) ? res ^ 1).

no functional change
bench: 1767398
Copy link

clang-format 18 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/noble/clang-format-18.

(execution 13074986246 / attempt 1)

@mstembera
Copy link
Contributor

Nice find! BTW I thought to ask if this works even if one side has multiple queens correct?

@pb00068
Copy link
Contributor Author

pb00068 commented Feb 3, 2025

Nice find! BTW I thought to ask if this works even if one side has multiple queens correct?

I think in a 'bench 16 1 20' there should be enough of this kind of positions.
If you have doubts, you can add such a position in benchmark.cpp and verify if the bench changes.
Anyhow I'm pretty sure its completely irrelevant if one side has multiple queens.

@Disservin Disservin added the to be merged Will be merged shortly label Feb 4, 2025
@Disservin Disservin closed this in 2a1ab11 Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-functional-change simplification A simplification patch to be merged Will be merged shortly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants