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 unneeded operator overload macros #4966

Closed
wants to merge 1 commit into from

Conversation

miguel-l
Copy link
Contributor

@miguel-l miguel-l commented Jan 5, 2024

Only Direction type is using two of the enable overload macros. Aside from this, only two of the overloads are even being used.

Therefore, we can just define the needed overloads and remove the macros.

No functional change.

Only Direction type is using two of the enable overload macros.
Aside from this, only two of the overloads are even being used.

Therefore, we can just define the needed overloads and remove the macros.

No functional change.
@Disservin
Copy link
Member

looks good, some would probably argue it's good to have these for the future but I'd much rather remove them and then discuss the readdition of operator overloads when something pops up

@Sopel97
Copy link
Member

Sopel97 commented Jan 5, 2024

The scope of operators should be defined by the type's semantics, not what's currently being used.

@miguel-l
Copy link
Contributor Author

miguel-l commented Jan 5, 2024

I think it's okay semantically to get rid of these as well. The macros are very specific to ints and I'm not sure stuff like subtracting or dividing a Direction by an int makes sense. Most existing overloads already handle semantics better (e.g. there are overloads for Square and Direction operations).

As for the ones I couldn't fully get rid of:

  • For operator+, we actually don't need it. It was being (mis?)used for Up+Up in pawn push calculations. I've replaced the overload with one that accepts 2 directions instead of a direction and an int.

  • operator* I could maybe make some sense of, but I'm really not sure. I'd probably remove it if I could but it's being used in a weird way for setting positions from fens.

@Disservin
Copy link
Member

Disservin commented Jan 6, 2024

operator* I could maybe make some sense of, but I'm really not sure. I'd probably remove it if I could but it's being used in a weird way for setting positions from fens.

The 2 * SOUTH is a bit weird, because it means "go south two times when you encounter a /",
but it makes sense because on the last char before a / we increment it by 1, which essentially goes to the start of the previous row/rank and from that we go south two times.
I think we can just leave that as is?

Here's the commit which added those btw 822695d

@miguel-l
Copy link
Contributor Author

miguel-l commented Jan 7, 2024

The 2 * SOUTH is a bit weird, because it means "go south two times when you encounter a /", but it makes sense because on the last char before a / we increment it by 1, which essentially goes to the start of the previous row/rank and from that we go south two times. I think we can just leave that as is?

Yes, I left operator* on int as it is

@Disservin Disservin added the to be merged Will be merged shortly label Jan 7, 2024
@Disservin Disservin closed this in a5f7386 Jan 7, 2024
Joachim26 pushed a commit to Joachim26/StockfishNPS that referenced this pull request Jan 14, 2024
Only Direction type is using two of the enable overload macros.
Aside from this, only two of the overloads are even being used.

Therefore, we can just define the needed overloads and remove the macros.

closes official-stockfish#4966

No functional change.
rn5f107s2 pushed a commit to rn5f107s2/Stockfish that referenced this pull request Jan 14, 2024
Only Direction type is using two of the enable overload macros.
Aside from this, only two of the overloads are even being used.

Therefore, we can just define the needed overloads and remove the macros.

closes official-stockfish#4966

No functional change.
windfishballad pushed a commit to windfishballad/Stockfish that referenced this pull request Jan 23, 2024
Only Direction type is using two of the enable overload macros.
Aside from this, only two of the overloads are even being used.

Therefore, we can just define the needed overloads and remove the macros.

closes official-stockfish#4966

No functional change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-bench-change to be merged Will be merged shortly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants