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

Reformat code in little-endian patch #3016

Merged
merged 1 commit into from
Aug 17, 2020

Conversation

snicolet
Copy link
Member

@snicolet snicolet commented Aug 16, 2020

Reformat code and rename the function to "read_little_endian()" in the recent
commit by Ronald de Man for support of big endian systems.

closes #3016

No functional change

@syzygy1
Copy link
Contributor

syzygy1 commented Aug 16, 2020

The cast relies on implementation-defined behaviour, which is why I used memcpy(). Of course any sane compiler implementation will leave the underlying bits unaffected...

@vondele
Copy link
Member

vondele commented Aug 16, 2020

yes, I agree that the use of memcpy here is the recommended way of doing this. I think our CI might pick this up.

@syzygy1
Copy link
Contributor

syzygy1 commented Aug 16, 2020

@snicolet
Copy link
Member Author

Fine with me (except if there is a retinterpret_blah_blah_cast() in the C++ standard for doing that, of course)..

@syzygy1
Copy link
Contributor

syzygy1 commented Aug 16, 2020

No, reinterpret_cast can't be used for this, I tried :-). Apparently that is meant for pointers.

@syzygy1
Copy link
Contributor

syzygy1 commented Aug 16, 2020

Well, perhaps one could reinterpret_cast a pointer to the unsigned value as a pointer to a signed value and then read it out...

@vondele
Copy link
Member

vondele commented Aug 16, 2020

I think memcpy is just fine for now. The compiler will replace the call, there is no performance impact, and right now this is a relatively clean way to do things.

@snicolet
Copy link
Member Author

snicolet commented Aug 16, 2020

I will squash the two commits into one.

Done, patch is ready IMO.

snicolet added a commit to snicolet/Stockfish that referenced this pull request Aug 16, 2020
Reformat code and rename the function to "read_little_endian()" in the recent
commit by Ronald de Man for support of big endian systems.

closes official-stockfish/Stockfish#3016

No functional change
@gvreuls
Copy link
Contributor

gvreuls commented Aug 17, 2020

Well, perhaps one could reinterpret_cast a pointer to the unsigned value as a pointer to a signed value and then read it out...

That's implementation dependent behavior too. In C++ (as opposed to C) you can only read memory that has been initialized by a constructor for the correct type and the only way around this is std::memcpy. This also means that the use of unions for casting is implementation dependent in C++ (but not in C), which makes the Score <-> Value functions make_score (uses a cast) and both mg_value and eg_value (use unions) not perfectly portable C++ (they don't cause any problems on architectures and compilers we support though).

Reformat code and rename the function to "read_little_endian()" in the recent
commit by Ronald de Man for support of big endian systems.

closes official-stockfish/Stockfish#3016

No functional change
-----

Recommended net: https://tests.stockfishchess.org/api/nn/nn-82215d0fd0df.nnue
@snicolet snicolet closed this in 81d716f Aug 17, 2020
@snicolet snicolet merged commit 81d716f into official-stockfish:master Aug 17, 2020
@syzygy1
Copy link
Contributor

syzygy1 commented Aug 17, 2020

@gvreuls mg_value() and eg_value() have been the subject of debate :-) See #211 and #849.

@snicolet snicolet added the to be merged Will be merged shortly label Aug 17, 2020
@snicolet
Copy link
Member Author

Merged via 81d716f

lucabrivio pushed a commit to lucabrivio/Stockfish that referenced this pull request Aug 17, 2020
Reformat code and rename the function to "read_little_endian()" in the recent
commit by Ronald de Man for support of big endian systems.

closes official-stockfish/Stockfish#3016

No functional change
-----

Recommended net: https://tests.stockfishchess.org/api/nn/nn-82215d0fd0df.nnue
joergoster pushed a commit to joergoster/Stockfish-old that referenced this pull request Aug 18, 2020
Reformat code and rename the function to "read_little_endian()" in the recent
commit by Ronald de Man for support of big endian systems.

closes official-stockfish/Stockfish#3016

No functional change
-----

Recommended net: https://tests.stockfishchess.org/api/nn/nn-82215d0fd0df.nnue
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

4 participants