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

Use if instead of goto #3461

Closed
wants to merge 2 commits into from
Closed

Use if instead of goto #3461

wants to merge 2 commits into from

Conversation

ProkopRandacek
Copy link
Contributor

This PR inverts the if and removes goto in the generate_all function.
It should even skip calling more_than_one() when Type != EVASIONS evaluates to true, right? I'm new to this so sorry if I'm missing something obvious.

@BM123499
Copy link
Contributor

BM123499 commented May 12, 2021

@ProkopRandacek. Note that Type is a template instantiation. So Type != EVASIONS is calculated at compile time. So it will only check for more_than_one() if Type == EVASIONS (this last condition won't be checked at running time). I believe both codes are equivalent.

@Sopel97
Copy link
Member

Sopel97 commented May 12, 2021

Yea, this goto is very weird here. Good patch

@snicolet
Copy link
Member

snicolet commented May 15, 2021

@ProkopRandacek
At least a result for a small speed test would be good here (locally with parallel benches or on fishtest with non-regression bounds), please.

The reason is that we have the habit to never commit anything touching the hot path of Stockfish without at least one measurement, even if the change seems "obvious". Better safe 100% of the time than sorry 1% :-)

The only exception is if the compiled binaries are the same, which can be checked with md5 stockfish.exe for instance.

@vondele vondele added the to be merged Will be merged shortly label May 19, 2021
@vondele vondele closed this in 6b9a70a May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
to be merged Will be merged shortly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants