Tiny cleanups#63
Open
winapiadmin wants to merge 32 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors Position API to snake_case, migrates Chess960 castling metadata into per-position storage, introduces EP pawn masks and centralized INVALID_ARG_IF error handling, converts the between-squares table to a fixed 64×64, adds Doxygen across headers/impls, updates move I/O to use new accessors and unified error handling, and extends test coverage (including doctest main and a malformed-FEN fuzzer). ChangesPosition and API refactor
Estimated code review effort:
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4dfc386a93
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
printers.cpp (1)
68-92:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDoxygen comments do not match actual stream output.
The docs say Color prints
"w"/"b", PieceType prints"p/n/b/...", and CastlingRights prints"KQkq", but the implementation prints enum names like"WHITE","PAWN", and"WHITE_OO". Please align comments or output behavior to avoid misleading API docs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@printers.cpp` around lines 68 - 92, The Doxygen comments for operator<<(std::ostream&, const Color), operator<<(std::ostream&, const PieceType), and operator<<(std::ostream&, const CastlingRights) do not match the current output (they return full enum names via DescriptiveNameNotation); either update the comments or change the implementations to emit the short forms described in the docs. Fix this by editing the three operator<< overloads: for Color replace "WHITE"/"BLACK" with "w"/"b"; for PieceType replace full names ("PAWN","KNIGHT",...) with single-letter algebraic piece chars ("p","n","b","r","q","k" and appropriate tokens for NO_PIECE_TYPE/PIECE_TYPE_NB); for CastlingRights ensure the output matches "KQkq" style (or update its comment). Modify the DescriptiveNameNotation initialization entries used in these functions (or the surrounding Doxygen comments) so the documentation and actual output are consistent.position.cpp (1)
882-914:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winType mismatch:
Bitboardvariables assignedSquarevalues.The variables
cr_BOO,cr_BOOO,cr_WOO,cr_WOOOare declared asBitboardbut are assignedSquarevalues fromcastling_meta_[...].rook_start_*. Later these are used both as squares (compared against) and as bitmasks (ORed intocastling). The implicit conversion fromSquaretoBitboarddoes not produce a bitmask; it just stores the square index.The logic at lines 890-894 ORs these square values directly into
castling, which is incorrect—you need1ULL << cr_WOOto get the actual bitmask for that square.🐛 Proposed fix
template <typename PieceC, typename T> CastlingRights _Position<PieceC, T>::clean_castling_rights() const { - const Bitboard cr_BOO = castling_meta_[BLACK].rook_start_ks; - const Bitboard cr_BOOO = castling_meta_[BLACK].rook_start_qs; - const Bitboard cr_WOO = castling_meta_[WHITE].rook_start_ks; - const Bitboard cr_WOOO = castling_meta_[WHITE].rook_start_qs; + const Square sq_BOO = castling_meta_[BLACK].rook_start_ks; + const Square sq_BOOO = castling_meta_[BLACK].rook_start_qs; + const Square sq_WOO = castling_meta_[WHITE].rook_start_ks; + const Square sq_WOOO = castling_meta_[WHITE].rook_start_qs; + const Bitboard cr_BOO = 1ULL << sq_BOO; + const Bitboard cr_BOOO = 1ULL << sq_BOOO; + const Bitboard cr_WOO = 1ULL << sq_WOO; + const Bitboard cr_WOOO = 1ULL << sq_WOOO; Bitboard castling = 0; // mappings🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@position.cpp` around lines 882 - 914, The cr_BOO/cr_BOOO/cr_WOO/cr_WOOO variables are declared as Bitboard but initialized from Square indices, so they contain square numbers not bitmasks; change their initialization to bitmasks (e.g. Bitboard cr_WOO = 1ULL << castling_meta_[WHITE].rook_start_ks) so all uses (the conditional ORs into castling, the rank & occ checks, the white_castling/black_castling &= (cr_WOO | cr_WOOO) lines, and the final mapping tests like (castling & cr_WOO)) operate on proper bitboards rather than raw square indices; keep the variable names and types the same and only adjust the expression that builds these cr_* values.
🧹 Nitpick comments (1)
fwd_decl.h (1)
104-106: ⚡ Quick win
Boardalias conflicts with the “remove aliases” objective.Given issue
#61’s goal, keepingusing Board = Position;(and documenting it) preserves the overlap this PR is trying to reduce. Consider removing this alias or deprecating it with a clear migration path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fwd_decl.h` around lines 104 - 106, Remove the legacy alias by deleting the line "using Board = Position;" from fwd_decl.h and update all code and docs to use Position directly (search for Board symbol usages and replace with Position), or if you must maintain compatibility, mark the alias as deprecated with a clear comment and migration note pointing users to replace Board with Position and add compiler deprecation attributes where supported; ensure tests/build pass after replacing references.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@position.h`:
- Around line 644-649: The ternary in material_key() is inverted causing
file_of(ep_square()) to be called when ep_square() == SQ_NONE; change the
expression used for indexing zobrist::RandomEP so it uses FILE_NB when
ep_square() == SQ_NONE and file_of(ep_square()) otherwise (i.e., swap the two
branches of the ternary). Update the material_key() function so ep_square(),
FILE_NB, and file_of(...) are used in the corrected order to avoid calling
file_of(SQ_NONE).
In `@printers.h`:
- Around line 36-50: The header comments for the stream operators
(operator<<(std::ostream&, const Color), operator<<(std::ostream&, const
CastlingRights), the templated operator<<(std::ostream&, PieceC),
operator<<(std::ostream&, const Square), and operator<<(std::ostream&, const
PieceType)) are misleading: they document human-readable notation while
printers.cpp emits enum-token strings (e.g. "WHITE", "WHITE_OO", "PAWN"). Update
the doc comments in printers.h to accurately describe the actual output format
(enum token names produced by the implementation) or, if you prefer
human-readable output, change the corresponding implementations in printers.cpp
to emit the documented strings; be sure the comment text references the exact
operator signatures above so future readers find the right overloads.
---
Outside diff comments:
In `@position.cpp`:
- Around line 882-914: The cr_BOO/cr_BOOO/cr_WOO/cr_WOOO variables are declared
as Bitboard but initialized from Square indices, so they contain square numbers
not bitmasks; change their initialization to bitmasks (e.g. Bitboard cr_WOO =
1ULL << castling_meta_[WHITE].rook_start_ks) so all uses (the conditional ORs
into castling, the rank & occ checks, the white_castling/black_castling &=
(cr_WOO | cr_WOOO) lines, and the final mapping tests like (castling & cr_WOO))
operate on proper bitboards rather than raw square indices; keep the variable
names and types the same and only adjust the expression that builds these cr_*
values.
In `@printers.cpp`:
- Around line 68-92: The Doxygen comments for operator<<(std::ostream&, const
Color), operator<<(std::ostream&, const PieceType), and
operator<<(std::ostream&, const CastlingRights) do not match the current output
(they return full enum names via DescriptiveNameNotation); either update the
comments or change the implementations to emit the short forms described in the
docs. Fix this by editing the three operator<< overloads: for Color replace
"WHITE"/"BLACK" with "w"/"b"; for PieceType replace full names
("PAWN","KNIGHT",...) with single-letter algebraic piece chars
("p","n","b","r","q","k" and appropriate tokens for
NO_PIECE_TYPE/PIECE_TYPE_NB); for CastlingRights ensure the output matches
"KQkq" style (or update its comment). Modify the DescriptiveNameNotation
initialization entries used in these functions (or the surrounding Doxygen
comments) so the documentation and actual output are consistent.
---
Nitpick comments:
In `@fwd_decl.h`:
- Around line 104-106: Remove the legacy alias by deleting the line "using Board
= Position;" from fwd_decl.h and update all code and docs to use Position
directly (search for Board symbol usages and replace with Position), or if you
must maintain compatibility, mark the alias as deprecated with a clear comment
and migration note pointing users to replace Board with Position and add
compiler deprecation attributes where supported; ensure tests/build pass after
replacing references.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 89064de1-0e8d-48e6-898d-df2f13205946
📒 Files selected for processing (15)
attacks.cppattacks.hbitboard.hfwd_decl.hmovegen.cppmovegen.hmoves_io.cppmoves_io.hposition.cppposition.hprinters.cppprinters.htypes.hzobrist.cppzobrist.h
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
moves_io.cpp (1)
367-384:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
remove_illegalslogic is ineffective: try/catch compiled only when exceptions aren't thrown.The
#if !defined(_CHESSLIB_ERROR_MODE_THROW)block includes try/catch to handle parsing errors, but in non-throw modeINVALID_ARG_IFeither asserts or is a no-op—no exceptions are thrown. Meanwhile, in throw mode where exceptions are thrown, this entire block is skipped andremove_illegalsis ignored.The test in
non_core_tests.cppexpectsparseSan(b, "0-0+?!", true)to succeed by trimming illegal characters, but this only works when exceptions are thrown and caught.🔧 Suggested approach
The try/catch trimming logic should be compiled when in throw mode:
-#if !defined(_CHESSLIB_ERROR_MODE_THROW) +#if defined(_CHESSLIB_ERROR_MODE_THROW) if (remove_illegals) { std::string trimmed_san(raw_san); while (!trimmed_san.empty()) { try { Move attempt = do_parse(trimmed_san); if (attempt.is_ok()) return attempt; } catch (...) { } trimmed_san.pop_back(); } INVALID_ARG_IF(trimmed_san.empty(), IllegalMoveException("illegal san: '" + std::string(raw_san) + "' in " + pos.fen())); return Move::none(); - } else -#endif + } +#else + (void)remove_illegals; // unused in non-throw modes +#endif return do_parse(raw_san);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@moves_io.cpp` around lines 367 - 384, The trimming-and-retry logic for remove_illegals is currently inside the `#if` !defined(_CHESSLIB_ERROR_MODE_THROW) guard so it never runs when exceptions are enabled; move or duplicate the try/catch-and-trim loop that calls do_parse(trimmed_san) (and returns attempt on attempt.is_ok()) out of the non-throw-only block so it is compiled in throw mode too, preserving the final INVALID_ARG_IF(trimmed_san.empty(), IllegalMoveException(...)) behavior and returning Move::none() if trimming fails; ensure remove_illegals, do_parse, INVALID_ARG_IF and the existing return do_parse(raw_san) paths remain correct.position.cpp (1)
861-899:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFix
is_insufficient_material(Color c)to usec(currently ignored)
position.cpp’s_Position<...>::is_insufficient_material(Color c)(lines 861-899) never referencescand instead evaluates insufficient material for the entire position. This conflicts withposition.h, where the API is per-color (is_insufficient_material(Color c) const) and the no-arg overload combines color-specific results viais_insufficient_material(WHITE) && is_insufficient_material(BLACK)(lines 569-570). Either make the implementation actually depend oncor remove the misleading parameter/doc (and add coverage whereWHITEvsBLACKshould differ).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@position.cpp` around lines 861 - 899, The implementation currently ignores the Color parameter c and computes insufficient-material over the whole board; fix it to evaluate material only for the given side by replacing global/white/black-specific bitboard uses with side-specific ones: use popcount(occ(c)) (or occ(c) | king if your convention includes the king) instead of popcount(occ()), replace white_bishops/black_bishops with bishops = pieces(BISHOP, c) and opponent_bishops = pieces(BISHOP, opposite(c)) only where you explicitly need opponent info, and change the knight checks to pieces(KNIGHT, c) (and similar for popcount(bishops) tests using lsb/msb on bishops). Update the count==4 branch to perform same-square-color checks using the side-specific bishop bitboard (and if you need the two-bishop-opposite-sides special case, explicitly read the opponent bitboard via pieces(BISHOP, opposite(c))). Ensure all references to WHITE/BLACK in is_insufficient_material are replaced with uses of c or opposite(c) as appropriate.
🧹 Nitpick comments (2)
printers.cpp (1)
114-117: 💤 Low valueDead code:
str_toupperis no longer used.The
str_toupperhelper function is defined but never called in this file. TheSquareprinter now usesuci::squareToString(sq)directly instead.🧹 Suggested removal
-static std::string str_toupper(std::string s) { - std::transform(s.begin(), s.end(), s.begin(), [](unsigned char c) { return std::toupper(c); }); - return s; -}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@printers.cpp` around lines 114 - 117, Remove the dead helper function str_toupper: it is defined as static std::string str_toupper(std::string) but is never referenced (Square printer now uses uci::squareToString(sq)). Delete the entire function definition to eliminate dead code and unused static linkage; run a quick build to ensure no remaining references to str_toupper remain.position.cpp (1)
48-59: 💤 Low valueMisleading comment about side to move.
The comment states "WHITE stm → shift down to rank 5" for RANK_6, but an EP square on RANK_6 actually means Black just moved (pushed a pawn from rank 7 to rank 5), so White is to move. The code logic is correct, but the comment is confusing.
📝 Suggested comment fix
Bitboard m = 1ULL << static_cast<int>(sq); if (r == RANK_6) - m >>= 8; // WHITE stm → shift down to rank 5 + m >>= 8; // EP on rank 6 → adjacent pawns on rank 5 else - m <<= 8; // BLACK stm → shift up to rank 4 + m <<= 8; // EP on rank 3 → adjacent pawns on rank 4🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@position.cpp` around lines 48 - 59, The comment near the ep_pawn_mask_for function is misleading about side to move: when r == RANK_6 the en-passant square is on rank 6 because Black just moved a pawn (so White is to move), and when r == RANK_3 it's because White just moved (so Black is to move); update the inline comment(s) around the shifts in ep_pawn_mask_for (and any adjacent comment mentioning "WHITE stm" or "BLACK stm") to correctly state which side just moved and which side is to move, e.g., clarify that RANK_6 corresponds to Black having moved (White to move) and RANK_3 corresponds to White having moved (Black to move) while leaving the existing shift logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@moves_io.cpp`:
- Around line 83-85: The default case in the switch uses INVALID_ARG_IF(false,
std::runtime_error(...)) so the error never fires; change the condition to true
so the invalid-castling-target path actually throws (i.e., replace the false
argument in the INVALID_ARG_IF call in that default branch with true), ensuring
the runtime_error("This isn't Chess960") is triggered for the invalid case.
- Around line 222-228: The check for short SAN (san.size() < 2) calls
INVALID_ARG_IF but doesn't return, so when INVALID_ARG_IF is a no-op control
falls through and san[san.size()-2] / san[san.size()-1] are accessed; update the
branch in moves_io.cpp to return Move::none() (same sentinel used later)
immediately after the INVALID_ARG_IF(... IllegalMoveException("illegal san: '" +
_san + "' in " + pos.fen())) call so that both throw/assert and non-throw/no-op
modes safely exit; target the block that uses san, INVALID_ARG_IF, _san,
pos.fen(), and Move::none().
In `@position.cpp`:
- Around line 315-318: The default branch currently uses INVALID_ARG_IF(false,
std::runtime_error("Invalid FEN character")) which never fires; replace this
with an unconditional error raise so the default case always fails: either call
the unconditional macro/utility (e.g., INVALID_ARG or equivalent) with
std::runtime_error("Invalid FEN character") or directly throw
std::runtime_error("Invalid FEN character") in the default: branch where the
INVALID_ARG_IF is used to ensure invalid FEN characters trigger an error; update
the default case in position.cpp accordingly.
- Around line 35-41: The macro guard names in the INVALID_ARG_IF definition are
inconsistent with the header: the code checks for _CHESSLIB_ERROR_MODE_THROW and
_CHESSLIB_ERROR_MODE_ASSERT but position.h uses _CHESSLIB_ERROR_MODE_THROWS and
_CHESSLIB_ERROR_MODE_ASSERTS, so the macro can end up undefined; fix by updating
the conditional macros to match the header names (or accept both spellings) —
e.g., change the `#if` defined(...) checks to use _CHESSLIB_ERROR_MODE_THROWS and
_CHESSLIB_ERROR_MODE_ASSERTS (or add additional defined(...) branches) so
INVALID_ARG_IF is compiled when those modes are active, keeping the existing
INVALID_ARG_IF, throw(...) and assert(...) behavior.
In `@position.h`:
- Around line 442-447: The error-checking block uses an undefined identifier
`sq` while the function parameter is named `s`; update the checks in the
preprocessor branches to use the actual parameter `s` (i.e., replace `sq` with
`s`) so both assert in the _CHESSLIB_ERROR_MODE_ASSERTS branch and the runtime
check/throw in _CHESSLIB_ERROR_MODE_THROWS refer to the correct symbol; locate
this in the function that takes parameter `s` in position.h and make the
substitution.
In `@types.h`:
- Around line 558-561: The operator[] currently only asserts the upper bound and
accepts a signed int, allowing negative indexes; update the check in inline T
&operator[](int index) to validate both bounds (assert(index >= 0 && index <
size_)) or change the parameter type to the container's unsigned size_type and
assert only the upper bound accordingly, so negative indexes cannot cause
undefined behavior when accessing values_.
---
Outside diff comments:
In `@moves_io.cpp`:
- Around line 367-384: The trimming-and-retry logic for remove_illegals is
currently inside the `#if` !defined(_CHESSLIB_ERROR_MODE_THROW) guard so it never
runs when exceptions are enabled; move or duplicate the try/catch-and-trim loop
that calls do_parse(trimmed_san) (and returns attempt on attempt.is_ok()) out of
the non-throw-only block so it is compiled in throw mode too, preserving the
final INVALID_ARG_IF(trimmed_san.empty(), IllegalMoveException(...)) behavior
and returning Move::none() if trimming fails; ensure remove_illegals, do_parse,
INVALID_ARG_IF and the existing return do_parse(raw_san) paths remain correct.
In `@position.cpp`:
- Around line 861-899: The implementation currently ignores the Color parameter
c and computes insufficient-material over the whole board; fix it to evaluate
material only for the given side by replacing global/white/black-specific
bitboard uses with side-specific ones: use popcount(occ(c)) (or occ(c) | king if
your convention includes the king) instead of popcount(occ()), replace
white_bishops/black_bishops with bishops = pieces(BISHOP, c) and
opponent_bishops = pieces(BISHOP, opposite(c)) only where you explicitly need
opponent info, and change the knight checks to pieces(KNIGHT, c) (and similar
for popcount(bishops) tests using lsb/msb on bishops). Update the count==4
branch to perform same-square-color checks using the side-specific bishop
bitboard (and if you need the two-bishop-opposite-sides special case, explicitly
read the opponent bitboard via pieces(BISHOP, opposite(c))). Ensure all
references to WHITE/BLACK in is_insufficient_material are replaced with uses of
c or opposite(c) as appropriate.
---
Nitpick comments:
In `@position.cpp`:
- Around line 48-59: The comment near the ep_pawn_mask_for function is
misleading about side to move: when r == RANK_6 the en-passant square is on rank
6 because Black just moved a pawn (so White is to move), and when r == RANK_3
it's because White just moved (so Black is to move); update the inline
comment(s) around the shifts in ep_pawn_mask_for (and any adjacent comment
mentioning "WHITE stm" or "BLACK stm") to correctly state which side just moved
and which side is to move, e.g., clarify that RANK_6 corresponds to Black having
moved (White to move) and RANK_3 corresponds to White having moved (Black to
move) while leaving the existing shift logic unchanged.
In `@printers.cpp`:
- Around line 114-117: Remove the dead helper function str_toupper: it is
defined as static std::string str_toupper(std::string) but is never referenced
(Square printer now uses uci::squareToString(sq)). Delete the entire function
definition to eliminate dead code and unused static linkage; run a quick build
to ensure no remaining references to str_toupper remain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6710ea4b-2847-41c6-ba8f-bd01cbdaff7a
📒 Files selected for processing (10)
CMakeLists.txtchess960_tests.cppfwd_decl.hmoves_io.cppposition.cppposition.hprinters.cppprinters.htests.cpptypes.h
✅ Files skipped from review due to trivial changes (1)
- printers.h
👮 Files not reviewed due to content moderation or server errors (1)
- chess960_tests.cpp
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
types.h (1)
600-610:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix pipeline warning: narrowing conversion from
size_ttoint.The loop variable
iissize_tandpisint. This triggers MSVC warning C4267 across multiple CI builds.🔧 Proposed fix
constexpr PieceType parse_pt(unsigned char c) { const char a[] = "pnbrqk"; - int p = -1; + int p = -1; if (c >= 'A' && c <= 'Z') c += 32; - for (size_t i = 0; i < sizeof(a); i++) { + for (int i = 0; i < static_cast<int>(sizeof(a)); i++) { if (c == a[i]) p = i; } return static_cast<PieceType>(p + 1); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@types.h` around lines 600 - 610, The loop in parse_pt uses size_t i while p is int, causing a narrowing warning; change the loop to use a signed int (or make p the same unsigned type) and iterate up to a signed length (e.g., cast sizeof(a) to int or compute an int length) so comparisons and assignment to p are the same type; update the loop variable in parse_pt and ensure the returned static_cast<PieceType>(p + 1) logic is unchanged.non_core_tests.cpp (1)
434-460:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPreprocessor guard logic is inverted.
The tests using
REQUIRE_THROWS_WITH_ASandCHECK_THROWS_ASare wrapped with#if !defined(_CHESSLIB_ERROR_MODE_THROW), meaning they only compile when throw mode is disabled. However, when throw mode is disabled,INVALID_ARG_IFbecomes a no-op (or an assert), so no exceptions are thrown and these tests will fail or be meaningless.The condition should be inverted: exception-assertion tests should only run when
_CHESSLIB_ERROR_MODE_THROWis defined.🐛 Proposed fix
-#if !defined(_CHESSLIB_ERROR_MODE_THROW) +#if defined(_CHESSLIB_ERROR_MODE_THROW) TEST_CASE("Test King Castling Short move with Annotation") { ... } TEST_CASE("Test King Castling Long move with Annotation") { ... } -#endif +#endif ... -#if !defined(_CHESSLIB_ERROR_MODE_THROW) +#if defined(_CHESSLIB_ERROR_MODE_THROW) TEST_CASE("Should throw on ambiguous move") { ... } TEST_CASE("Should throw for illegal move") { ... } -#endif +#endifAlso applies to: 883-903
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@non_core_tests.cpp` around lines 434 - 460, The preprocessor guards around the exception tests are inverted: change the conditional from `#if` !defined(_CHESSLIB_ERROR_MODE_THROW) to `#if` defined(_CHESSLIB_ERROR_MODE_THROW) so tests that use REQUIRE_THROWS_WITH_AS / CHECK_THROWS_AS only compile when exceptions are enabled; update both the castling test block (where uci::parseSan is expected to throw) and the other occurrence mentioned (lines 883-903) to use the defined(...) guard instead of !defined(...), leaving the test bodies and symbols (Move::make, Move::none, uci::parseSan, chess::uci::IllegalMoveException) unchanged.
🧹 Nitpick comments (2)
types.h (1)
559-561: 💤 Low valueUpper bound check uses
MaxSizeinstead ofsize_.The assertion now checks
index < MaxSize(capacity) rather thanindex < size_(current element count). This allows accessing uninitialized elements betweensize_andMaxSize. If this is intentional for performance reasons, consider adding a comment; otherwise, usesize_for bounds checking.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@types.h` around lines 559 - 561, The bounds check currently asserts index < MaxSize which validates against the container capacity instead of the number of initialized elements; change the assertion in the accessor to assert(0 <= index && index < size_) to prevent accessing uninitialized entries (symbols: index, MaxSize, size_, values_). If the current behavior was intentional for performance or SSO reasons, add a clear comment above the assert explaining why accessing up-to-capacity is allowed and document any invariant guaranteeing values_[index] is valid up to MaxSize.moves_io.cpp (1)
369-386: ⚖️ Poor tradeoffThe
remove_illegalsfeature is unavailable when_CHESSLIB_ERROR_MODE_THROWis defined.When
_CHESSLIB_ERROR_MODE_THROWis defined, theremove_illegalsparameter is ignored anddo_parse(raw_san)is always called directly (line 386). This means users cannot use the annotation-stripping fallback logic in throw mode.Additionally, in non-throw mode, the
try-catchblock (lines 373-378) is dead code sinceINVALID_ARG_IFbecomes a no-op anddo_parsereturnsMove::none()instead of throwing.Consider restructuring so
remove_illegalsworks regardless of error mode, using a success/failure return pattern rather than exceptions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@moves_io.cpp` around lines 369 - 386, The remove_illegals trimming logic must run regardless of _CHESSLIB_ERROR_MODE_THROW; refactor so do_parse is invoked in a non-throwing/checked way (or wrap it in a local try/catch) inside the remove_illegals branch instead of being bypassed by the preprocessor. Specifically, implement a checked parse flow (e.g., a try_parse/parse_optional pattern or a local try/catch around do_parse) used by the trimming loop that returns a Move or a failure flag so the while-loop can test attempt.is_ok() without relying on exceptions; only after exhausting trimming call INVALID_ARG_IF/throw or return Move::none() as appropriate. Update references in moves_io.cpp for remove_illegals, do_parse, INVALID_ARG_IF, and Move::none() so the trimming fallback works in both throw and non-throw modes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@non_core_tests.cpp`:
- Around line 434-460: The preprocessor guards around the exception tests are
inverted: change the conditional from `#if` !defined(_CHESSLIB_ERROR_MODE_THROW)
to `#if` defined(_CHESSLIB_ERROR_MODE_THROW) so tests that use
REQUIRE_THROWS_WITH_AS / CHECK_THROWS_AS only compile when exceptions are
enabled; update both the castling test block (where uci::parseSan is expected to
throw) and the other occurrence mentioned (lines 883-903) to use the
defined(...) guard instead of !defined(...), leaving the test bodies and symbols
(Move::make, Move::none, uci::parseSan, chess::uci::IllegalMoveException)
unchanged.
In `@types.h`:
- Around line 600-610: The loop in parse_pt uses size_t i while p is int,
causing a narrowing warning; change the loop to use a signed int (or make p the
same unsigned type) and iterate up to a signed length (e.g., cast sizeof(a) to
int or compute an int length) so comparisons and assignment to p are the same
type; update the loop variable in parse_pt and ensure the returned
static_cast<PieceType>(p + 1) logic is unchanged.
---
Nitpick comments:
In `@moves_io.cpp`:
- Around line 369-386: The remove_illegals trimming logic must run regardless of
_CHESSLIB_ERROR_MODE_THROW; refactor so do_parse is invoked in a
non-throwing/checked way (or wrap it in a local try/catch) inside the
remove_illegals branch instead of being bypassed by the preprocessor.
Specifically, implement a checked parse flow (e.g., a try_parse/parse_optional
pattern or a local try/catch around do_parse) used by the trimming loop that
returns a Move or a failure flag so the while-loop can test attempt.is_ok()
without relying on exceptions; only after exhausting trimming call
INVALID_ARG_IF/throw or return Move::none() as appropriate. Update references in
moves_io.cpp for remove_illegals, do_parse, INVALID_ARG_IF, and Move::none() so
the trimming fallback works in both throw and non-throw modes.
In `@types.h`:
- Around line 559-561: The bounds check currently asserts index < MaxSize which
validates against the container capacity instead of the number of initialized
elements; change the assertion in the accessor to assert(0 <= index && index <
size_) to prevent accessing uninitialized entries (symbols: index, MaxSize,
size_, values_). If the current behavior was intentional for performance or SSO
reasons, add a clear comment above the assert explaining why accessing
up-to-capacity is allowed and document any invariant guaranteeing values_[index]
is valid up to MaxSize.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: de8632a2-d500-4f01-b904-d0a47e39503b
📒 Files selected for processing (5)
moves_io.cppnon_core_tests.cppposition.cppposition.htypes.h
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@moves_io.cpp`:
- Around line 83-85: The default branch in the castling handling inside
moveToUci() currently only invokes INVALID_ARG_IF and can fall through in no-op
error mode, producing malformed UCI; update that default case (the switch's
default handling for Chess960 castling inside moveToUci) to explicitly return a
sentinel value (for example an empty string or other agreed-upon invalid-uci
marker) immediately after the INVALID_ARG_IF call so the function cannot
continue and append more data when the castling is invalid.
- Around line 148-152: The current code uses INVALID_ARG_IF to detect illegal
moves but then unconditionally returns move, which becomes a no-op when
_CHESSLIB_ERROR_MODE disables validation; update uciToMove's validation after
calling pos.legals(moves) by explicitly checking the iterator (auto it =
std::find(moves.begin(), moves.end(), move)) and when it == moves.end() return
Move::NO_MOVE (while preserving the existing INVALID_ARG_IF call so
exceptions/asserts still fire in those modes); otherwise return move. This
touches Movelist, pos.legals, the iterator check, INVALID_ARG_IF and the
returned move.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b1164f0e-7a05-4761-a4b2-64b10b2ea96a
📒 Files selected for processing (3)
moves_io.cppnon_core_tests.cpptests.cpp
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests.cpp`:
- Around line 20-22: Replace the non-portable check that defines
DOCTEST_CONFIG_NO_EXCEPTIONS_BUT_WITH_ALL_ASSERTS based only on __EXCEPTIONS;
instead use a portable feature-detection guard such as checking multiple
exception macros (for example: `#if` !defined(__cpp_exceptions) &&
!defined(__EXCEPTIONS) && !defined(_CPPUNWIND)) and then define
DOCTEST_CONFIG_NO_EXCEPTIONS_BUT_WITH_ALL_ASSERTS inside that block so both
tests.cpp and non_core_tests.cpp detect disabled exceptions across
compilers/builds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 29343cf5-9565-4bf5-a696-445428d9e3bb
📒 Files selected for processing (5)
CMakeLists.txtchess960_tests.cppmoves_io.cppnon_core_tests.cpptests.cpp
👮 Files not reviewed due to content moderation or server errors (1)
- chess960_tests.cpp
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
fwd_decl.h (1)
106-106: ⚡ Quick winAdd an explicit deprecation message for
Board.Use
[[deprecated("Use Position instead")]]so compiler warnings give direct migration guidance.Proposed change
-using Board [[deprecated]] = Position; +using Board [[deprecated("Use Position instead")]] = Position;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fwd_decl.h` at line 106, Replace the bare deprecated attribute on the alias Board with an explicit message to guide users: change the declaration of the type alias Board (currently "using Board [[deprecated]] = Position;") to use the form [[deprecated("Use Position instead")]] so compiler warnings show the migration hint; update the alias declaration in fwd_decl.h where Board is defined to use that message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@fwd_decl.h`:
- Line 106: Replace the bare deprecated attribute on the alias Board with an
explicit message to guide users: change the declaration of the type alias Board
(currently "using Board [[deprecated]] = Position;") to use the form
[[deprecated("Use Position instead")]] so compiler warnings show the migration
hint; update the alias declaration in fwd_decl.h where Board is defined to use
that message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ac7d1d7-4e17-4e4a-bc66-3444e8d0d49a
📒 Files selected for processing (7)
CMakeLists.txtchess960_tests.cppfwd_decl.hmoves_io.cppnon_core_tests.cpptests.cpptypes.h
🚧 Files skipped from review as they are similar to previous changes (5)
- CMakeLists.txt
- tests.cpp
- non_core_tests.cpp
- types.h
- moves_io.cpp
👮 Files not reviewed due to content moderation or server errors (1)
- chess960_tests.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@non_core_tests.cpp`:
- Around line 153-158: The test's try/catch around constructing _Position<T>
swallows exceptions and prevents the REQUIRE(pos.was_into_check() == tc.info)
from running; remove the try/catch so constructor exceptions propagate and fail
the test, or if some inputs are expected to throw, replace the catch with an
explicit assertion (e.g., use REQUIRE_THROWS or mark those cases) so failures
are not silently ignored—look for the _Position<T> constructor usage and the
catch block surrounding REQUIRE to apply the change.
- Around line 20-22: The preprocessor condition in non_core_tests.cpp inverts
exception detection and defines
DOCTEST_CONFIG_NO_EXCEPTIONS_BUT_WITH_ALL_ASSERTS when exceptions are present;
change the check so the macro is defined only when no exception macros are
defined. Update the conditional that references __cpp_exceptions, _CPPUNWIND,
__EXCEPTIONS, and _CHESSLIB_ERROR_MODE_THROW to require all of them to be absent
(i.e., negate each and combine with &&) before defining
DOCTEST_CONFIG_NO_EXCEPTIONS_BUT_WITH_ALL_ASSERTS so doctest’s exception-less
mode matches tests.cpp.
- Line 1066: Replace the incorrect call p.set_fen(test.fen) with the existing
API p.setFEN(test.fen) (or add a thin wrapper named set_fen that forwards to
setFEN) so the code uses the defined Position API; update the call in
non_core_tests.cpp to use setFEN and ensure the signature and argument
(test.fen) match the implementation in position.h and tests.cpp.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b671a3cf-2c23-4022-a03e-b6e0a8c2b345
📒 Files selected for processing (5)
fwd_decl.hnon_core_tests.cppposition.cppposition.htests.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- fwd_decl.h
- position.h
- position.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
position.cpp (1)
949-987:⚠️ Potential issue | 🟠 Major | ⚡ Quick winParameter
cis unused — function ignores the color argument.The signature was changed to
is_insufficient_material(Color c), but the implementation still checks material for both colors unconditionally. The parametercis never referenced, making the function behave identically regardless of which color is passed.Either use the parameter to check only that color's material, or revert to the parameterless signature.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@position.cpp` around lines 949 - 987, The function _Position<PieceC, T>::is_insufficient_material currently ignores the Color c parameter; change the implementation to use c by computing counts and piece bitboards relative to that side (e.g., use occ(c) or popcount(occ(c)) for the side's piece count, and pieces(PAWN/KING/BISHOP/KNIGHT, c) for that side and pieces(..., other) for the opponent where needed), and replace hard-coded white_bishops/black_bishops checks with pieces(BISHOP, c) and pieces(BISHOP, other) (derive other via the existing Color inversion/utility); alternatively, if the function should be global, revert the signature to remove the unused Color c — but do one or the other so the parameter is not unused.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@position.cpp`:
- Around line 604-606: The branch that skips refresh_attacks() for non-standard
FENs leaves cached attack state stale; when popcount(pieces(WHITE, KING)) != 1
or popcount(pieces(BLACK, KING)) != 1 you must explicitly clear the cached
attack fields instead of leaving them uninitialized. In the same block where
refresh_attacks() is skipped (the if/return area around refresh_attacks()), set
_checkers and _check_mask to zero and reset/clear _rook_pin and _bishop_pin (or
their container/state equivalents) so subsequent calls to legals(), is_check(),
is_valid(), or king_sq() (which may return SQ_NONE) do not read stale data.
Ensure the reset uses the same types/clear methods used elsewhere in the class
so invariants remain consistent.
---
Outside diff comments:
In `@position.cpp`:
- Around line 949-987: The function _Position<PieceC,
T>::is_insufficient_material currently ignores the Color c parameter; change the
implementation to use c by computing counts and piece bitboards relative to that
side (e.g., use occ(c) or popcount(occ(c)) for the side's piece count, and
pieces(PAWN/KING/BISHOP/KNIGHT, c) for that side and pieces(..., other) for the
opponent where needed), and replace hard-coded white_bishops/black_bishops
checks with pieces(BISHOP, c) and pieces(BISHOP, other) (derive other via the
existing Color inversion/utility); alternatively, if the function should be
global, revert the signature to remove the unused Color c — but do one or the
other so the parameter is not unused.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a44f82f0-29db-4402-aeb5-5abd97d75d89
📒 Files selected for processing (5)
chess960_tests.cppnon_core_tests.cppposition.cppposition.htests.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- tests.cpp
- non_core_tests.cpp
👮 Files not reviewed due to content moderation or server errors (1)
- chess960_tests.cpp
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…into tiny_cleanup
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
position.cpp (1)
953-990:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle the remaining dead 4-piece endgames here.
The
count == 4branch still misses several insufficient-material cases, e.g. K+N vs K+N, K+B vs K+N, and K+B vs K+B with opposite-colored bishops. Those positions currently returnfalse, so draw detection through this new no-argument API will be wrong.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@position.cpp` around lines 953 - 990, The count == 4 branch in _Position<PieceC, T>::is_insufficient_material currently only handles same-colored double-bishop cases but misses K+N vs K+N, K+B vs K+N and K+B vs K+B with opposite-colored bishops; update that branch to explicitly detect and return true for these remaining dead 4-piece endgames by: check for two-knight-only material (both sides have exactly one KNIGHT and no other minor pieces) and return true for K+N vs K+N; check for one side having exactly one BISHOP and the other exactly one KNIGHT (pieces(BISHOP, X) and pieces(KNIGHT, Y) with no other minors) and return true for K+B vs K+N; and for K+B vs K+B verify bishop-square color parity (use the existing ((9*(sq1 ^ sq2)) & 8) test on lsb/msb of white_bishops/black_bishops) and return true when bishops are on opposite colors; use popcount, pieces(KNIGHT,...), pieces(BISHOP,...) and lsb/msb/Square helpers to locate the pieces and keep these checks inside the count == 4 branch before falling back to return false.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@position.cpp`:
- Around line 953-990: The count == 4 branch in _Position<PieceC,
T>::is_insufficient_material currently only handles same-colored double-bishop
cases but misses K+N vs K+N, K+B vs K+N and K+B vs K+B with opposite-colored
bishops; update that branch to explicitly detect and return true for these
remaining dead 4-piece endgames by: check for two-knight-only material (both
sides have exactly one KNIGHT and no other minor pieces) and return true for K+N
vs K+N; check for one side having exactly one BISHOP and the other exactly one
KNIGHT (pieces(BISHOP, X) and pieces(KNIGHT, Y) with no other minors) and return
true for K+B vs K+N; and for K+B vs K+B verify bishop-square color parity (use
the existing ((9*(sq1 ^ sq2)) & 8) test on lsb/msb of
white_bishops/black_bishops) and return true when bishops are on opposite
colors; use popcount, pieces(KNIGHT,...), pieces(BISHOP,...) and lsb/msb/Square
helpers to locate the pieces and keep these checks inside the count == 4 branch
before falling back to return false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c44d7699-cda6-43b3-bfed-f6a3b9feda68
📒 Files selected for processing (2)
position.cppposition.h
🚧 Files skipped from review as they are similar to previous changes (1)
- position.h
This was
linked to
issues
May 28, 2026
…into tiny_cleanup
…into tiny_cleanup
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
moves_io.cpp (1)
31-53: ⚡ Quick winSingle-source
INVALID_ARG_IFinstead of duplicating it.This helper is still defined independently here and in
position.cpp, so future error-mode changes can drift between parsing and position-state code. If the goal is centralized validation behavior, move it to one internal header.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@moves_io.cpp` around lines 31 - 53, Duplicate macro definitions of INVALID_ARG_IF exist (seen in moves_io.cpp and position.cpp); centralize the helper into a single internal header and remove the duplicate in this file. Create or use an internal header (e.g., internal/validation.h) that defines INVALID_ARG_IF with the same conditional branches, include that header in moves_io.cpp and position.cpp, and delete the local macro definitions here so all code shares one canonical implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test.yml:
- Around line 45-46: The CI matrix defined by cpp_compiler and error_mode is
producing too many permutations; reduce jobs by shrinking those axes and/or
switching to explicit combinations: update the matrix block so cpp_compiler no
longer lists all three compilers for every job and error_mode doesn't iterate
all three modes across every compiler—either limit cpp_compiler to 1
representative compiler per OS and test all error_mode values, or keep all
compilers but test only a single representative error_mode, or replace the broad
axes with an explicit matrix.include list of targeted combos; modify the matrix
keys (cpp_compiler, error_mode, matrix.include/exclude) accordingly to produce a
much smaller, representative set of jobs.
- Around line 45-50: The CI matrix includes cpp_compiler values g++ and clang++
that are not preinstalled on Windows runners, causing the Configure CMake step
(which sets CMAKE_CXX_COMPILER) to fail; either update the matrix to exclude g++
and clang++ for windows-latest (keep only cl) or add explicit installation steps
for MinGW/LLVM on Windows before the Configure CMake job so g++/clang++ exist at
runtime; make the change around the cpp_compiler matrix entries and the
Configure CMake step that sets CMAKE_CXX_COMPILER.
In `@position.cpp`:
- Around line 46-56: INVALID_ARG_IF currently becomes a no-op in release builds
which allows invalid moves to reach state-mutating code (push_uci -> parse_uci
-> do_move -> _Position::doMove). Modify INVALID_ARG_IF so it enforces failures
instead of only logging: when (c) is true, throw the provided exception (or call
std::terminate/abort if exceptions are undesirable) so callers like
_Position::doMove() cannot continue with an invalid move; update the macro
definition used in release builds to perform throw exception; keep the same
macro name and parameter usage so calls in parse_uci(), push_uci(), and
_Position::doMove() remain unchanged.
- Around line 1002-1020: The branch treating four-piece endings uses total minor
counts (popcount(knights)/popcount(bishops)) and thus misclassifies
side-specific mate-capable material; change it to per-side checks: compute own
and opponent minor counts (e.g., own_knights = knights & us_bitboard,
their_knights = knights & them_bitboard; same for bishops) and then evaluate
K+N+N vs K, K+B+N vs K and K+B+B vs K based on one side having both minors and
the other having none; when checking same-colored bishops use
square_color(lsb(own_bishops)) == square_color(msb(own_bishops)) on the owning
side rather than comparing global bishops. Ensure all return true/false
decisions reference these per-side counts instead of the global
popcount(knights)/popcount(bishops).
---
Nitpick comments:
In `@moves_io.cpp`:
- Around line 31-53: Duplicate macro definitions of INVALID_ARG_IF exist (seen
in moves_io.cpp and position.cpp); centralize the helper into a single internal
header and remove the duplicate in this file. Create or use an internal header
(e.g., internal/validation.h) that defines INVALID_ARG_IF with the same
conditional branches, include that header in moves_io.cpp and position.cpp, and
delete the local macro definitions here so all code shares one canonical
implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: eb40c177-1268-4f4d-b9d1-86598ed41ede
📒 Files selected for processing (6)
.github/workflows/test.yml.github/workflows/try_compile.ymlmoves_io.cppposition.cppprinters.cpptypes.h
💤 Files with no reviewable changes (1)
- .github/workflows/try_compile.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- printers.cpp
- types.h
Comment on lines
+45
to
+46
| cpp_compiler: [g++, clang++, cl] | ||
| error_mode: [ASSERT, THROW, NONE] |
Contributor
There was a problem hiding this comment.
Consider reducing the matrix combinations to optimize CI resources.
The matrix now generates approximately 30 jobs per PR (2 OS × 2 build types × 3 compilers × 3 error modes, minus exclusions). This is a significant increase from the original ~4 jobs and will substantially increase CI time and resource consumption. Consider whether all combinations are necessary, or if a representative subset would provide sufficient coverage (e.g., test all error modes with one compiler per OS, and test all compilers with one error mode).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/test.yml around lines 45 - 46, The CI matrix defined by
cpp_compiler and error_mode is producing too many permutations; reduce jobs by
shrinking those axes and/or switching to explicit combinations: update the
matrix block so cpp_compiler no longer lists all three compilers for every job
and error_mode doesn't iterate all three modes across every compiler—either
limit cpp_compiler to 1 representative compiler per OS and test all error_mode
values, or keep all compilers but test only a single representative error_mode,
or replace the broad axes with an explicit matrix.include list of targeted
combos; modify the matrix keys (cpp_compiler, error_mode,
matrix.include/exclude) accordingly to produce a much smaller, representative
set of jobs.
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
1 participant
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.
Fixes #61 #60 #59 #64 #65 #66
Adds docstrings
Caution
Non-backwards-compatible