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

Remove operators for color #2247

Closed

Conversation

protonspring
Copy link

@protonspring protonspring commented Jul 16, 2019

This is a non-functional and untested simplification. The increment operator for color isn't really necessary and seems a bit unnatural to me.

@snicolet
Copy link
Member

This needs some test to check that there is not speed penalty, depending on what code the compiler emits for the alternate form..

@protonspring
Copy link
Author

I tried to use godbolt, but the compiler doesn't seem to recognize the new form.

Are you wanting a -3,1 test on the framework, or godbolt code?

@Sopel97
Copy link
Member

Sopel97 commented Jul 20, 2019

https://godbolt.org/z/XmG1wL
tldr: if you don't care about msvc then it's the same codegen. clang has stupidly high limit on unrolling so pretty much no problem

@snicolet
Copy link
Member

A -3,1 test in the framework would be fine

@Rocky640
Copy link

@protonspring
Before running a [-3, 1] test, while being there, can you also prettify the following line
for (CastlingSide s = KING_SIDE; s <= QUEEN_SIDE; s = CastlingSide(s + 1))
for
for (CastlingSide s : { KING_SIDE, QUEEN_SIDE })

@protonspring
Copy link
Author

STC
LLR: 2.95 (-2.94,2.94) [-3.00,1.00]
Total: 47027 W: 10589 L: 10518 D: 25920
http://tests.stockfishchess.org/tests/view/5d3472d10ebc5925cf0e8d3e

31m059 referenced this pull request in protonspring/Stockfish Jul 24, 2019
@snicolet
Copy link
Member

Could we retest the PR version in isolation? thanks :-)

@protonspring
Copy link
Author

protonspring commented Jul 25, 2019

@snicolet I'm not sure what you're asking for. You want a -3,1 test without Rocky's addition?

@Rocky640
Copy link

The proposed change to CastlingSide in bool function pos_is_ok is harmless.
pos_is_ok is only called in debug mode and inside some assert calls

@snicolet
Copy link
Member

snicolet commented Jul 26, 2019

@protonspring @Rocky640
I was confused when I saw the commit "single kingSquares", which was not intented to be part of the pull request if I understand correctly. I am fine with the changes for Color and CastlingSide, which is indeed not in the critical path of SF.

@snicolet snicolet closed this in aec918a Jul 26, 2019
@snicolet
Copy link
Member

Merged via aec918a

@Rocky640
Copy link

@snicolet That was busy day !

mstembera pushed a commit to mstembera/Stockfish that referenced this pull request Jul 31, 2019
This is a non-functional and untested simplification. The increment operator
for color isn't really necessary and seems a bit unnatural to me.

Passed STC:
LLR: 2.95 (-2.94,2.94) [-3.00,1.00]
Total: 47027 W: 10589 L: 10518 D: 25920
http://tests.stockfishchess.org/tests/view/5d3472d10ebc5925cf0e8d3e

Closes official-stockfish#2247

No functional change
mstembera pushed a commit to mstembera/Stockfish that referenced this pull request Aug 1, 2019
This is a non-functional and untested simplification. The increment operator
for color isn't really necessary and seems a bit unnatural to me.

Passed STC:
LLR: 2.95 (-2.94,2.94) [-3.00,1.00]
Total: 47027 W: 10589 L: 10518 D: 25920
http://tests.stockfishchess.org/tests/view/5d3472d10ebc5925cf0e8d3e

Closes official-stockfish#2247

No functional change
mstembera pushed a commit to mstembera/Stockfish that referenced this pull request Aug 29, 2019
This is a non-functional and untested simplification. The increment operator
for color isn't really necessary and seems a bit unnatural to me.

Passed STC:
LLR: 2.95 (-2.94,2.94) [-3.00,1.00]
Total: 47027 W: 10589 L: 10518 D: 25920
http://tests.stockfishchess.org/tests/view/5d3472d10ebc5925cf0e8d3e

Closes official-stockfish#2247

No functional change
pb00068 pushed a commit to pb00068/Stockfish that referenced this pull request Sep 10, 2019
This is a non-functional and untested simplification. The increment operator
for color isn't really necessary and seems a bit unnatural to me.

Passed STC:
LLR: 2.95 (-2.94,2.94) [-3.00,1.00]
Total: 47027 W: 10589 L: 10518 D: 25920
http://tests.stockfishchess.org/tests/view/5d3472d10ebc5925cf0e8d3e

Closes official-stockfish#2247

No functional change
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.

None yet

4 participants