search

niklasf / shakmaty

A Rust library for chess and chess variant rules and operations
https://docs.rs/shakmaty
GNU General Public License v3.0
210 stars 41 forks source link

`Move` could be `Copy`. #48

Closed KarelPeeters closed 2 years ago

KarelPeeters commented 2 years ago

The Move enum has a simple definition, and is very small: when I compile the crate it's only 8 bytes/64 bits. It would be very convenient if it derived the Copy trait so we don't need to call .clone or work with references anywhere. I can't really think of any disadvantages.

niklasf commented 2 years ago

The disadvantage I had in mind is that it prevents accidentally using the same move twice.

I tried the same with Piece, so that putting a piece on the board consumes it, but that was just too annoying to work with. I didn't find the same for Move. Will you really need a lot of cloning?

Pushed a copy-move branch to experiment.

KarelPeeters commented 2 years ago

Yeah I guess that's true, and not deriving Copy for types where it's actually dangerous if they are used twice makes sense. But intuitively a chess move is not some "heavy" object, it's just a small atomic thing. I was surprised when I started using this crate so that's why I made this issue.

The Copy docs say this:

Generally speaking, if your type can implement Copy, it should. Keep in mind, though, that implementing Copy is part of the public API of your type. If the type might become non-Copyin the future, it could be prudent to omit the Copy implementation now, to avoid a breaking API change.

(My specific use case is that I'm trying to replace the chess backend chess crate used in https://github.com/KarelPeeters/board-game-rs with shakmatey because the chess crate takes a long time to compile, and there I started out by assuming that moves are Copy. I might change this back to not requiring copy later on for more complex games, so it's not super important for me.)

niklasf commented 2 years ago

Keeping !Copy for now.