search

jordanbray / chess

A rust library to manage chess move generation
https://jordanbray.github.io/chess/
MIT License
245 stars 57 forks source link

Fix UB (issue 52) #59

Closed terrorfisch closed 1 year ago

terrorfisch commented 3 years ago

See #52

I additionally replaced the transmute with a match statement which results in a no-op with minimal optimization but might slow down debug builds. Tested with godbolt.

terrorfisch commented 3 years ago

I additionally defined the representations of CastleRights, Color and Piece because they are used to index into arrays with get_unchecked in several places.

analog-hors commented 3 years ago

I don't believe that's actually needed because of the indexing, since casting an enum with as should be perfectly fine. To my knowledge, the only use of transmute happens with Rank and File. Now that it's a match instead, the repr(u8) is probably also not needed for those enums too.

terrorfisch commented 3 years ago

My fault, I misremembered that the values are guaranteed.

As in C, discriminant values that are not specified are defined as either 0 (for the first variant) or as one more than the prior variant.

jordanbray commented 3 years ago

This looks good to me. I don't see any reason to avoid merging. Is this ready to go?

terrorfisch commented 3 years ago

If you do not mind the revert commit this can be merged.

jordanbray commented 3 years ago

I'll probably do a squash and merge. I generally prefer that.

terrorfisch commented 3 years ago

Then the other question is if you want to keep the repr(u8) because the current code does not need it anymore. The only downside is imo that it would be a breaking change to remove it again. The upside is that users can transmute slices without UB.

jordanbray commented 3 years ago

I don't really think I care either way. repr(u8) seems like it would allow mem::transmute, but I doubt it's that useful in the first place. Any time I can imagine needing a slice of Rank or File I feel like I'd intuitively reach for a BitBoard instead.

analog-hors commented 3 years ago

I don't think there's any point in keeping the repr

terrorfisch commented 3 years ago

I think one should allow users of this library to transmute if they want to but that is not a strong opinion. @jordanbray You decide and I open up a new squashed PR. Or do you want to do it?