Code-quality pass: deduplicate move-making/primitives and tighten lints - #324
Merged
Conversation
- make_move: extract toggle_piece/move_piece helpers that keep the Zobrist hash in sync, collapsing ~12 repetitions of the manual bitboard-mutate-plus-hash-xor pattern. Determine the moving piece once via Pieces::at and dispatch, instead of running every move through four sub-functions that each re-scan the bitboards. - Piece: make the algebraic-notation letter the single source of truth via PieceKind::to_char/from_char, replacing the two 12-arm char<->Piece matches; derive Copy/Clone/Eq for Piece. - Castling generation: encapsulate the static castle geometry in a const Castle table and iterate, removing the 9-argument try_castle helper and the parallel 9-tuple destructure. - UCI: carry a Go parameter struct instead of an 8-field command variant and an 8-argument Engine::go; extract Engine::limits. parse_go builds the struct with a shared millisecond parser. - Add Bitboard::toggle and BitXorAssign to complete the operator set. Behaviour is unchanged: full perft suite (incl. the shakmaty differential test), the incremental-hash invariant test, and UCI integration tests all pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GVrKvkMe4xDmkRAksacBzk
- Enable the rustc lints that were commented out and already pass clean: unreachable_pub, trivial_numeric_casts, unused_qualifications, unused_import_braces, unused_lifetimes, unused_extern_crates, macro_use_extern_crate and absolute_paths_not_starting_with_crate. Keep missing_docs / let_underscore_drop / unused_results deferred with a note, since they still fire on undocumented API and the search stubs. - Enable the core clippy groups: correctness (deny), complexity and style (warn), alongside the existing perf/suspicious denies. pedantic, nursery and cargo stay off — they are dominated by literal-separator noise and transitive-dependency version reports. - Adopt rustfmt style_edition = "2024". - Simplify the FEN piece-placement parser with the ? operator (surfaced by the newly enabled clippy::style group). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GVrKvkMe4xDmkRAksacBzk
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A quality-focused pass over the chess core and the tooling config. No behaviour changes — purely deduplication, modern idioms, and stricter lints. Verified against the full perft suite (including the shakmaty differential test), the incremental-hash invariant test, the UCI integration tests, and a self-play smoke game.
Refactoring (
refactor:commit)make_move: the Zobrist hash-update boilerplate (self.hash ^= generated::get_piece_key(Piece { player, kind }, square)) was repeated ~12 times across four sub-functions, each of which re-scanned the piece bitboards to find the mover. Introducedtoggle_piece/move_piecehelpers that keep the hash in sync, and determine the moving piece once viaPieces::at, then dispatch. Shorter and does less work per move (the old code ran every move through all four sub-functions).char↔Piecematches with a single source of truth,PieceKind::to_char/from_char;Piecenow derivesCopy/Clone/Eq.const Castletable and iterate over it, deleting the 9-argumenttry_castlehelper and its parallel 9-tuple destructure.gocommand now carries aGoparameter struct instead of an 8-field enum variant feeding an 8-argumentEngine::go; extractedEngine::limits. Both "too many arguments" clippy smells are gone, and the parser tests shrank via..Go::default().Bitboardoperator set withtoggle+BitXorAssign.Harness (
chore:commit)You mentioned the lint config hadn't been revisited in a while — I measured the churn of every commented-out lint and turned on the ones that are already clean:
unreachable_pub,trivial_numeric_casts,unused_qualifications,unused_import_braces,unused_lifetimes,unused_extern_crates,macro_use_extern_crate,absolute_paths_not_starting_with_crate(all +0 warnings today). Leftmissing_docs/let_underscore_drop/unused_resultsdeferred with a note — they still fire on undocumented public API and the search stubs.correctness(deny), pluscomplexityandstyle(warn), alongside the existingperf/suspiciousdenies.pedantic(+844),nursery(+26) andcargostay off — dominated by literal-separator noise and transitive-dependency version reports, not real issues.style_edition = "2024".stylefinding this surfaced (match→?in the FEN parser).Testing
cargo test: 130 tests + doctests pass.cargo test --release -- --ignored: all 12 expensive perft / shakmaty-differential tests pass (re-run after the castling and make/move refactors).cargo +nightly fmt --checkclean;cargo clippy --all-targets --all-featuresclean of denied lints.🤖 Generated with Claude Code
https://claude.ai/code/session_01GVrKvkMe4xDmkRAksacBzk
Generated by Claude Code