bugfixes#55
Open
winapiadmin wants to merge 14 commits intomainfrom
Open
Conversation
|
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:
📝 WalkthroughWalkthroughGPL headers added across many files; Direction enum extended with DOUBLE_* values and shift updated; null-move checks inserted into perft/test flow; argument-check macros made debug-only; fifty/seventyfive-move APIs corrected to parameterless and thresholds fixed. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Tests::perft
participant Gen as MoveGenerator
participant Pos as Position
participant Null as NullMove
Test->>Gen: generate_moves(pos)
loop for each move
Test->>Pos: doMove(move)
Pos->>Null: doNullMove()
Null-->>Pos: temporary side-to-move/state change
Test->>Test: recurse perft(depth-1)
Test->>Null: undoNullMove()
Null-->>Pos: restore side-to-move/state
Test->>Pos: undoMove(move)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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)
Comment |
|
Note Docstrings generation - SUCCESS |
coderabbitai Bot
added a commit
that referenced
this pull request
Apr 30, 2026
Docstrings generation was requested by @winapiadmin. * #55 (comment) The following files were modified: * `moves_io.cpp` * `position.h` * `tests.cpp` * `types.h`
![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: dc8a2a64a4
ℹ️ 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}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
attacks.cpp (1)
164-164:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove implicit narrowing on
entry.index = offset.Line 164 triggers C4267 (
size_t→int). Make the conversion explicit and range-checked, or widenMagic::index.Proposed local fix
+#include <limits> ... - entry.index = offset; + ASSUME(offset <= static_cast<size_t>(std::numeric_limits<int>::max())); + entry.index = static_cast<int>(offset);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@attacks.cpp` at line 164, The assignment entry.index = offset implicitly narrows a size_t to Magic::index (int) and triggers C4267; fix by either widening Magic::index to size_t or performing an explicit, range-checked conversion where offset is assigned to entry.index (check offset <= std::numeric_limits<Magic::index>::max() and handle overflow) so the conversion is safe and unambiguous; locate the assignment to entry.index and update the type or add the explicit check/cast accordingly.attacks.h (1)
166-187:⚠️ Potential issue | 🟠 MajorHandle newly added
Directionenum values inshift()to preventUNREACHABLE()crashes.The
Directionenum now includes 9 values not handled by theshift()function:DOUBLE_NORTH,DOUBLE_EAST,DOUBLE_SOUTH,DOUBLE_WEST,DOUBLE_NORTH_EAST,DOUBLE_SOUTH_EAST,DOUBLE_SOUTH_WEST,DOUBLE_NORTH_WEST, andDIR_NONE. Any call toshift()with these values will hit thedefaultcase and abort atUNREACHABLE().Suggested fix
switch (direction) { case Direction::NORTH: return b << 8; case Direction::SOUTH: return b >> 8; case Direction::NORTH_WEST: return (b & ~MASK_FILE[0]) << 7; case Direction::WEST: return (b & ~MASK_FILE[0]) >> 1; case Direction::SOUTH_WEST: return (b & ~MASK_FILE[0]) >> 9; case Direction::NORTH_EAST: return (b & ~MASK_FILE[7]) << 9; case Direction::EAST: return (b & ~MASK_FILE[7]) << 1; case Direction::SOUTH_EAST: return (b & ~MASK_FILE[7]) >> 7; +case Direction::DOUBLE_NORTH: + return b << 16; +case Direction::DOUBLE_SOUTH: + return b >> 16; +case Direction::DOUBLE_EAST: + return b << 2; +case Direction::DOUBLE_WEST: + return b >> 2; +case Direction::DOUBLE_NORTH_EAST: + return (b & ~MASK_FILE[7]) << 18; +case Direction::DOUBLE_SOUTH_EAST: + return (b & ~MASK_FILE[7]) >> 14; +case Direction::DOUBLE_SOUTH_WEST: + return (b & ~MASK_FILE[0]) >> 18; +case Direction::DOUBLE_NORTH_WEST: + return (b & ~MASK_FILE[0]) << 14; +case Direction::DIR_NONE: + return b; default: UNREACHABLE(); return 0; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@attacks.h` around lines 166 - 187, The shift() switch must handle the new Direction enum values to avoid the default UNREACHABLE(); add cases for DIR_NONE (return the input bitboard unchanged) and for each DOUBLE_* value (DOUBLE_NORTH, DOUBLE_EAST, DOUBLE_SOUTH, DOUBLE_WEST, DOUBLE_NORTH_EAST, DOUBLE_SOUTH_EAST, DOUBLE_SOUTH_WEST, DOUBLE_NORTH_WEST) implement them by applying the single-step shift twice (e.g., return shift(shift(b, EAST), EAST) or the corresponding single-step direction) so masking logic is reused and no new bit-math is duplicated; keep all existing single-step cases as-is and remove reliance on the default UNREACHABLE() for these new enums.non_core_tests.cpp (1)
20-22:⚠️ Potential issue | 🟠 MajorParser failures are silently masked when exceptions are disabled, preventing proper test validation.
Lines 20–22 conditionally disable exceptions when
__EXCEPTIONSis not defined. When this occurs,parseSan()usesTHROW_IF_EXCEPTIONS_ON, which becomes a no-op((void)0)instead of throwingIllegalMoveExceptionorAmbiguousMoveException. This causes invalid SAN input to silently returnMove::none()rather than producing an exception, allowing tests likeREQUIRE(parseSan(b, "fxe5") == m)to fail silently instead of catching parser bugs.Note:
tests.cppandchess960_tests.cppunconditionally defineDOCTEST_CONFIG_NO_EXCEPTIONS_BUT_WITH_ALL_ASSERTS(without the__EXCEPTIONSguard), meaning exceptions are always disabled in those files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@non_core_tests.cpp` around lines 20 - 22, The tests are masking parser failures because when __EXCEPTIONS is undefined the macro THROW_IF_EXCEPTIONS_ON becomes a no-op so parseSan() returns Move::none() instead of raising IllegalMoveException or AmbiguousMoveException; to fix this, ensure parser errors still fail tests by making THROW_IF_EXCEPTIONS_ON behave consistently: either stop conditionally defining DOCTEST_CONFIG_NO_EXCEPTIONS_BUT_WITH_ALL_ASSERTS (remove the __EXCEPTIONS guard) so exceptions remain enabled for tests that rely on them, or change the macro implementation so that when exceptions are disabled it invokes a non-throwing failure path (e.g., call a test-failure helper or abort) inside parseSan()/the parsing code so IllegalMoveException/AmbiguousMoveException conditions still cause a hard test failure rather than silently returning Move::none(); update references to parseSan(), THROW_IF_EXCEPTIONS_ON, IllegalMoveException, AmbiguousMoveException and tests that call parseSan(b, "...") accordingly.position.h (2)
196-218:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard attack refresh against sentinel history moves.
Lines 196/200 now refresh after every undo, while Line 216 stores
Move::null()in history. CI is already asserting inMove::from_sq(), so the refresh path still assumesstate().mv.is_ok(). Please special-caseMove::none()/Move::null()here, or makerefresh_attacks()rebuild purely from board state instead of the last move.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@position.h` around lines 196 - 218, The undo/null-move path in doNullMove pushes a sentinel Move::null() into history then calls refresh_attacks(), but refresh_attacks() currently assumes state().mv.is_ok() (it triggers Move::from_sq() CI); to fix, update doNullMove (or the undo/refresh callers) to special-case sentinel moves by setting state().mv to Move::none()/an invalid marker before calling refresh_attacks() or by passing a flag to refresh_attacks() to rebuild attacks solely from board state when state().mv is null; modify doNullMove, any undo logic that inspects history, and refresh_attacks() to check Move::null()/Move::none() (and use state() board occupancy, piece lists, enPassant, castling, etc.) so refresh_attacks() never dereferences a sentinel move.
391-408:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInitialize
_p2before the debug reconstruction.If the square is occupied but none of the piece bitboards match,
_p2is read uninitialized in the assertion path. The current CI failure on Line 407 shows that this corrupted state is reachable, so the diagnostic itself has UB.Suggested fix
- PieceC _p2; + PieceC _p2 = PieceC::NO_PIECE;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@position.h` around lines 391 - 408, The variable _p2 can remain uninitialized if the square is occupied but no piece bitboard matches, causing undefined behavior in the assertion comparing pieces_list[s] to _p2; to fix, initialize _p2 to a safe default (e.g., PieceC::NO_PIECE) at declaration or set it to PieceC::NO_PIECE after the loop if no match was found, keeping the rest of the logic (state().occ, state().pieces, make_piece<PieceC>, pieces_list[s]) and the assertion intact.
🧹 Nitpick comments (1)
tests.cpp (1)
218-222: ⚡ Quick winAdd immediate round-trip invariants around the null-move stress step.
Right after each
doNullMove()/undoMove()pair, asserthash()andfen()are unchanged. This will localize the regression much earlier than a later deepdoMoveabort.Suggested test hardening
for (const Move &m : moves) { pos.template doMove<false>(m); + const auto pre_nm_hash_1 = pos.hash(); + const auto pre_nm_fen_1 = pos.fen(); pos.doNullMove(); pos.undoMove(); + REQUIRE(pos.hash() == pre_nm_hash_1); + REQUIRE(pos.fen() == pre_nm_fen_1); const uint64_t nodes = perft<T, mt, false>(pos, depth - 1); + const auto pre_nm_hash_2 = pos.hash(); + const auto pre_nm_fen_2 = pos.fen(); pos.doNullMove(); pos.undoMove(); + REQUIRE(pos.hash() == pre_nm_hash_2); + REQUIRE(pos.fen() == pre_nm_fen_2); pos.undoMove();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests.cpp` around lines 218 - 222, Add immediate invariants after each null-move round trip to catch regressions early: after calling pos.doNullMove() and pos.undoMove(), assert that pos.hash() and pos.fen() are unchanged compared to values captured just before the null-move; do this for both null-move pairs around the perft call (use temporary variables to store pre-null hash/fen and assert equality after undoMove()). Ensure assertions reference pos.doNullMove(), pos.undoMove(), pos.hash(), and pos.fen().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CMakeLists.txt`:
- Line 18: The CMake minimum version is too low: update the
cmake_minimum_required(VERSION ...) call to 3.14 because the project uses
features that require newer CMake—specifically
CMAKE_CXX_COMPILER_FRONTEND_VARIANT and add_link_options; change the
cmake_minimum_required invocation to 3.14 so these symbols/functions are
supported.
In `@moves_io.cpp`:
- Around line 264-270: When parse_square returns SQ_NONE the
THROW_IF_EXCEPTIONS_ON(...) is a no-op in builds without exceptions, causing
execution to fall through with src_square == SQ_NONE; fix by aborting the parse
there: after the THROW_IF_EXCEPTIONS_ON(IllegalMoveException(...)) call add an
explicit return Move::none() so the function exits immediately instead of
continuing to set has_src_square and proceeding with invalid state; adjust
around the parse_square/THROW_IF_EXCEPTIONS_ON usage that sets src_square and
has_src_square.
In `@moves_io.h`:
- Around line 19-22: Remove the unlicensed upstream attribution comment in
moves_io.h and replace it with an explicit license header or SPDX identifier for
this file; if you do not have permission to relicense the pjpuzzler/cpp-chess
code, delete the reference and either (a) add the appropriate GPL-3.0 (or other
chosen) license text/SPDX tag for this file and document the provenance and
permission, or (b) remove or replace any code derived from pjpuzzler/cpp-chess
with a properly licensed alternative (e.g., integrate python-chess under its
GPL-3.0 terms) and add the corresponding license/attribution; ensure the
file-level comment or header now contains the explicit license grant or SPDX
identifier instead of the current ambiguous line referencing
pjpuzzler/cpp-chess.
---
Outside diff comments:
In `@attacks.cpp`:
- Line 164: The assignment entry.index = offset implicitly narrows a size_t to
Magic::index (int) and triggers C4267; fix by either widening Magic::index to
size_t or performing an explicit, range-checked conversion where offset is
assigned to entry.index (check offset <=
std::numeric_limits<Magic::index>::max() and handle overflow) so the conversion
is safe and unambiguous; locate the assignment to entry.index and update the
type or add the explicit check/cast accordingly.
In `@attacks.h`:
- Around line 166-187: The shift() switch must handle the new Direction enum
values to avoid the default UNREACHABLE(); add cases for DIR_NONE (return the
input bitboard unchanged) and for each DOUBLE_* value (DOUBLE_NORTH,
DOUBLE_EAST, DOUBLE_SOUTH, DOUBLE_WEST, DOUBLE_NORTH_EAST, DOUBLE_SOUTH_EAST,
DOUBLE_SOUTH_WEST, DOUBLE_NORTH_WEST) implement them by applying the single-step
shift twice (e.g., return shift(shift(b, EAST), EAST) or the corresponding
single-step direction) so masking logic is reused and no new bit-math is
duplicated; keep all existing single-step cases as-is and remove reliance on the
default UNREACHABLE() for these new enums.
In `@non_core_tests.cpp`:
- Around line 20-22: The tests are masking parser failures because when
__EXCEPTIONS is undefined the macro THROW_IF_EXCEPTIONS_ON becomes a no-op so
parseSan() returns Move::none() instead of raising IllegalMoveException or
AmbiguousMoveException; to fix this, ensure parser errors still fail tests by
making THROW_IF_EXCEPTIONS_ON behave consistently: either stop conditionally
defining DOCTEST_CONFIG_NO_EXCEPTIONS_BUT_WITH_ALL_ASSERTS (remove the
__EXCEPTIONS guard) so exceptions remain enabled for tests that rely on them, or
change the macro implementation so that when exceptions are disabled it invokes
a non-throwing failure path (e.g., call a test-failure helper or abort) inside
parseSan()/the parsing code so IllegalMoveException/AmbiguousMoveException
conditions still cause a hard test failure rather than silently returning
Move::none(); update references to parseSan(), THROW_IF_EXCEPTIONS_ON,
IllegalMoveException, AmbiguousMoveException and tests that call parseSan(b,
"...") accordingly.
In `@position.h`:
- Around line 196-218: The undo/null-move path in doNullMove pushes a sentinel
Move::null() into history then calls refresh_attacks(), but refresh_attacks()
currently assumes state().mv.is_ok() (it triggers Move::from_sq() CI); to fix,
update doNullMove (or the undo/refresh callers) to special-case sentinel moves
by setting state().mv to Move::none()/an invalid marker before calling
refresh_attacks() or by passing a flag to refresh_attacks() to rebuild attacks
solely from board state when state().mv is null; modify doNullMove, any undo
logic that inspects history, and refresh_attacks() to check
Move::null()/Move::none() (and use state() board occupancy, piece lists,
enPassant, castling, etc.) so refresh_attacks() never dereferences a sentinel
move.
- Around line 391-408: The variable _p2 can remain uninitialized if the square
is occupied but no piece bitboard matches, causing undefined behavior in the
assertion comparing pieces_list[s] to _p2; to fix, initialize _p2 to a safe
default (e.g., PieceC::NO_PIECE) at declaration or set it to PieceC::NO_PIECE
after the loop if no match was found, keeping the rest of the logic
(state().occ, state().pieces, make_piece<PieceC>, pieces_list[s]) and the
assertion intact.
---
Nitpick comments:
In `@tests.cpp`:
- Around line 218-222: Add immediate invariants after each null-move round trip
to catch regressions early: after calling pos.doNullMove() and pos.undoMove(),
assert that pos.hash() and pos.fen() are unchanged compared to values captured
just before the null-move; do this for both null-move pairs around the perft
call (use temporary variables to store pre-null hash/fen and assert equality
after undoMove()). Ensure assertions reference pos.doNullMove(), pos.undoMove(),
pos.hash(), and pos.fen().
🪄 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: c1e0d068-989e-4b22-86c2-c8064d01940d
📒 Files selected for processing (19)
CMakeLists.txtattacks.cppattacks.hbitboard.hchess960_tests.cppfwd_decl.hmovegen.cppmovegen.hmoves_io.cppmoves_io.hnon_core_tests.cppposition.cppposition.hprinters.cppprinters.htests.cpptypes.hzobrist.cppzobrist.h
👮 Files not reviewed due to content moderation or server errors (1)
- chess960_tests.cpp
There was a problem hiding this comment.
♻️ Duplicate comments (1)
CMakeLists.txt (1)
18-18:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore the minimum CMake version.
cmake_minimum_required(VERSION 3.10)is now below the documented floor for features used in this file:add_link_options()needs 3.13+,FetchContent_MakeAvailable()needs 3.14+, andCMAKE_CXX_COMPILER_FRONTEND_VARIANTwas added in 3.14. That means thisCMakeLists.txtwill fail to configure on 3.10-3.13 builders. (cmake.org)Suggested fix
-cmake_minimum_required(VERSION 3.10) +cmake_minimum_required(VERSION 3.14)Run this read-only check to confirm the documented introduction versions:
#!/bin/bash set -euo pipefail cmake --help-command add_link_options | sed -n '1,25p' echo cmake --help-module FetchContent | sed -n '1,90p' echo cmake --help-variable CMAKE_CXX_COMPILER_FRONTEND_VARIANT | sed -n '1,40p'Also applies to: 25-49, 66-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` at line 18, The CMake minimum version is too low: update the cmake_minimum_required(...) call to at least 3.14 (or a version >=3.14) so features used—add_link_options, FetchContent_MakeAvailable/FetchContent module, and the CMAKE_CXX_COMPILER_FRONTEND_VARIANT variable—are available; locate the cmake_minimum_required line and bump it to a compatible version (e.g., 3.14 or newer) and ensure any other occurrences of cmake_minimum_required in the file are updated consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@CMakeLists.txt`:
- Line 18: The CMake minimum version is too low: update the
cmake_minimum_required(...) call to at least 3.14 (or a version >=3.14) so
features used—add_link_options, FetchContent_MakeAvailable/FetchContent module,
and the CMAKE_CXX_COMPILER_FRONTEND_VARIANT variable—are available; locate the
cmake_minimum_required line and bump it to a compatible version (e.g., 3.14 or
newer) and ensure any other occurrences of cmake_minimum_required in the file
are updated consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 310d254c-4df0-46f5-b98a-1feeb596eb15
📒 Files selected for processing (1)
CMakeLists.txt
…ount of output printed
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests.cpp`:
- Around line 216-236: The test currently captures heavy state snapshots
(pos.hash(), pos.fen(), pos.zobrist()) around pos.doNullMove()/pos.undoMove()
but the REQUIRE checks are commented out, so the fen()/zobrist() calls waste
perft time; fix by either removing the snapshot calls entirely from the hot path
or restoring the checks behind a debug-only gate (e.g. wrap the
pre_nm_hash_1/pre_nm_fen_1 snapshots and the REQUIREs in an NDEBUG/DEBUG_ASSERT
guard or a TESTING_VALIDATE macro) so they only execute in debug/test builds;
apply the exact same change to both blocks around the calls to
pos.doNullMove()/pos.undoMove() (the blocks that use pos.hash(), pos.fen(),
pos.zobrist()) and ensure perft<T, mt, false>(pos, depth - 1) remains
unaffected.
🪄 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: 1f09871f-a873-47da-ae2e-4028b4303097
📒 Files selected for processing (7)
CMakeLists.txtattacks.hmoves_io.cppmoves_io.hnon_core_tests.cppposition.htests.cpp
✅ Files skipped from review due to trivial changes (2)
- moves_io.h
- CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (3)
- attacks.h
- moves_io.cpp
- non_core_tests.cpp
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests.cpp (1)
229-240:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRelease perft path still pays snapshot cost without any active validation.
fen()/hash()are captured here but never asserted, so this adds hot-path overhead with no benefit. Keep only the null move roundtrip in release, or fully gate snapshots/assertions to debug builds.Suggested adjustment
`#if` IS_RELEASE { - const auto pre_nm_hash_1 = pos.hash(); - const auto pre_nm_fen_1 = pos.fen(); - // REQUIRE(pos.zobrist() == pre_nm_hash_1); pos.doNullMove(); pos.undoMove(); - /*REQUIRE(pos.hash() == pre_nm_hash_1); - REQUIRE(pos.fen() == pre_nm_fen_1); - REQUIRE(pos.zobrist() == pre_nm_hash_1);*/ } `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests.cpp` around lines 229 - 240, The release build is capturing pos.fen()/pos.hash()/pos.zobrist() but never asserting them, adding unnecessary runtime cost; change the block so in release you only do the null-move roundtrip (call pos.doNullMove() and pos.undoMove()) and remove the fen()/hash()/zobrist() calls, or alternatively wrap the snapshot captures and REQUIRE asserts in a debug-only guard (e.g., `#ifndef` NDEBUG or a DEBUG_BUILD macro) so only debug builds perform pos.fen(), pos.hash(), and pos.zobrist() and verify them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests.cpp`:
- Around line 229-240: The release build is capturing
pos.fen()/pos.hash()/pos.zobrist() but never asserting them, adding unnecessary
runtime cost; change the block so in release you only do the null-move roundtrip
(call pos.doNullMove() and pos.undoMove()) and remove the fen()/hash()/zobrist()
calls, or alternatively wrap the snapshot captures and REQUIRE asserts in a
debug-only guard (e.g., `#ifndef` NDEBUG or a DEBUG_BUILD macro) so only debug
builds perform pos.fen(), pos.hash(), and pos.zobrist() and verify them.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests.cpp`:
- Around line 224-228: The conditional guard around the post-null-move checks
weakens the test: instead of only running the three REQUIREs when all three
values differ, remove the conditional and assert pos.hash(), pos.fen(), and
pos.zobrist() unconditionally after the null-move round trip (i.e., always call
REQUIRE(pos.hash() == pre_nm_hash_1), REQUIRE(pos.fen() == pre_nm_fen_1),
REQUIRE(pos.zobrist() == pre_nm_hash_1) in the block that follows the null-move
restore, and do the same for the second occurrence that compares against
pre_nm_hash_2/pre_nm_fen_2), ensuring exact restoration is always verified for
pos.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
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
2 participants
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 #54 #53 #52
adds license notes