Improve documentation and optimize move generation logic#67
Merged
Conversation
… comments across position.h, types.h, attacks.h, fwd_decl.h, and moves_io.h to document public API and reach the 80% doc coverage target. Ran Doxygen and updated tools/missing.json.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…vailable for faster bishop/rook attacks Detect BMI2 at runtime (GCC/Clang __builtin_cpu_supports, MSVC CPUID) and use _pext_u64; fall back to multiply-and-shift. Rebuild and benchmarked perft: 17.15s (186.36 Mnps). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…erted runtime BMI2 detection changes to restore portable magic lookup used previously for universal binaries.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tures; remove unused code and improve clarity
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:
📝 WalkthroughWalkthroughAdds precomputed directional ray bitboards ( ChangesChess Engine Core and Infrastructure
Sequence Diagram(s)sequenceDiagram
participant refresh_attacks as refresh_attacks()
participant RAYS as RAYS[8]
participant scan as scan_attacks_ray
participant checkers as _checkers
participant pins as _bishop/_rook_pin
participant check_mask as _check_mask
refresh_attacks->>RAYS: load 4 diagonal rays from king
loop each diagonal direction
RAYS-->>refresh_attacks: ray bitboard
refresh_attacks->>scan: find 1st & 2nd occupied squares
scan-->>refresh_attacks: blocker bitboards
alt 1st blocker is enemy bishop/queen
refresh_attacks->>checkers: set bit
end
alt 2nd blocker is own piece
refresh_attacks->>pins: set bishop_pin bit
end
end
refresh_attacks->>RAYS: load 4 orthogonal rays from king
loop each orthogonal direction
RAYS-->>refresh_attacks: ray bitboard
refresh_attacks->>scan: find 1st & 2nd occupied squares
scan-->>refresh_attacks: blocker bitboards
alt 1st blocker is enemy rook/queen
refresh_attacks->>checkers: set bit
end
alt 2nd blocker is own piece
refresh_attacks->>pins: set rook_pin bit
end
end
refresh_attacks->>check_mask: derive from checkers popcount (0/1/>1)
refresh_attacks-->>refresh_attacks: copy constructor copies cached fields directly
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 876ee5f2f7
ℹ️ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
movegen.cpp (1)
36-67:⚠️ Potential issue | 🔴 CriticalAlign the AVX-512 guards with the intrinsics actually used.
The first guard at line 36 checks
__AVX512VNNI__and__AVX512VBMI__, but the block uses_mm512_maskz_compress_epi8,_mm512_cvtepi8_epi16, and_mm512_slli_epi16, which require AVX512VBMI2 and AVX512BW respectively. Similarly, the fallback at line 67 checks only__AVX512BW__but uses_mm512_maskz_compress_epi16, which requires AVX512VBMI2. An explicit-mavx512*build can select these paths while still missing required features. Update both guards to check all necessary feature macros: the first to include__AVX512BW__and__AVX512VBMI2__, and the second to include__AVX512VBMI2__.Suggested fix
-#if defined(__AVX512F__) && defined(__AVX512VNNI__) && defined(__AVX512VBMI__) +#if defined(__AVX512F__) && defined(__AVX512BW__) && defined(__AVX512VBMI2__) && defined(__AVX512VNNI__) && defined(__AVX512VBMI__) ... -#elif defined(__AVX512BW__) +#elif defined(__AVX512F__) && defined(__AVX512BW__) && defined(__AVX512VBMI2__)🤖 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 `@movegen.cpp` around lines 36 - 67, Update the preprocessor guards to match the actual AVX-512 intrinsics being used. The first guard starting with `#if defined(__AVX512F__)` at line 36 needs to also check for `__AVX512BW__` and `__AVX512VBMI2__` since the functions `splat_pawn_moves` and `splat_moves` use intrinsics like `_mm512_maskz_compress_epi8`, `_mm512_cvtepi8_epi16`, and `_mm512_slli_epi16` which require these features. The second fallback guard with `#elif defined(__AVX512BW__)` at line 67 needs to also check for `__AVX512VBMI2__` since it uses `_mm512_maskz_compress_epi16` which requires that feature.
🧹 Nitpick comments (3)
Doxyfile (1)
13-13: ⚡ Quick winEnable Doxygen warnings so docs CI can catch regressions.
Line 13 disables diagnostics, so malformed/invalid docs silently pass in CI. Prefer enabling warnings and failing on important warnings.
Suggested config tweak
-WARNINGS = NO +WARNINGS = YES +WARN_IF_UNDOCUMENTED = YES +WARN_AS_ERROR = YES🤖 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 `@Doxyfile` at line 13, The WARNINGS configuration option in Doxyfile is currently set to NO, which disables Doxygen diagnostics and allows malformed or invalid documentation to pass silently through CI. Change the WARNINGS setting from NO to YES to enable warnings and allow the CI pipeline to detect and fail on documentation regressions and issues.position.cpp (1)
966-967: 💤 Low valueConsider removing commented-out dead code.
These variables appear to be unused. Rather than leaving them commented out, consider deleting them entirely to keep the codebase clean.
🤖 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 966 - 967, Remove the two commented-out lines containing unused variable declarations for wb_cnt and bb_cnt in the position.cpp file. These lines compute popcount values that are never used elsewhere in the code, so delete them entirely rather than leaving them commented out to maintain code cleanliness and clarity.position.h (1)
226-228: 💤 Low valueDuplicated
@brieftags in Doxygen comments.Several methods have duplicated
@brieftags (e.g., lines 227-228, 254-255, 510-511, 521-522). While this doesn't affect runtime behavior, it may cause confusing Doxygen output. Consider removing the redundant tags.🤖 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.h` around lines 226 - 228, The Doxygen documentation comments for the do_move method contain duplicate `@brief` tags, with the same description appearing twice on consecutive lines. Remove one of the duplicate `@brief` tags so that there is only a single `@brief` description line before the template do_move method declaration. Apply this same fix to all other methods in the file that have duplicate `@brief` tags to ensure clean Doxygen output.
🤖 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/doxygen.yml:
- Around line 11-12: The actions/checkout action is referenced using a mutable
tag `@v4` at two locations in .github/workflows/doxygen.yml, creating supply-chain
risk. At line 11-12 (the Checkout step), replace the `@v4` tag with a pinned full
commit SHA and add persist-credentials: false to the uses line to prevent
credential leakage to subsequent workflow steps. At line 26, replace the `@v4` tag
with the same pinned full commit SHA for the actions/checkout action to ensure
consistency and security across all workflow invocations of third-party actions.
---
Outside diff comments:
In `@movegen.cpp`:
- Around line 36-67: Update the preprocessor guards to match the actual AVX-512
intrinsics being used. The first guard starting with `#if defined(__AVX512F__)`
at line 36 needs to also check for `__AVX512BW__` and `__AVX512VBMI2__` since
the functions `splat_pawn_moves` and `splat_moves` use intrinsics like
`_mm512_maskz_compress_epi8`, `_mm512_cvtepi8_epi16`, and `_mm512_slli_epi16`
which require these features. The second fallback guard with `#elif
defined(__AVX512BW__)` at line 67 needs to also check for `__AVX512VBMI2__`
since it uses `_mm512_maskz_compress_epi16` which requires that feature.
---
Nitpick comments:
In `@Doxyfile`:
- Line 13: The WARNINGS configuration option in Doxyfile is currently set to NO,
which disables Doxygen diagnostics and allows malformed or invalid documentation
to pass silently through CI. Change the WARNINGS setting from NO to YES to
enable warnings and allow the CI pipeline to detect and fail on documentation
regressions and issues.
In `@position.cpp`:
- Around line 966-967: Remove the two commented-out lines containing unused
variable declarations for wb_cnt and bb_cnt in the position.cpp file. These
lines compute popcount values that are never used elsewhere in the code, so
delete them entirely rather than leaving them commented out to maintain code
cleanliness and clarity.
In `@position.h`:
- Around line 226-228: The Doxygen documentation comments for the do_move method
contain duplicate `@brief` tags, with the same description appearing twice on
consecutive lines. Remove one of the duplicate `@brief` tags so that there is only
a single `@brief` description line before the template do_move method declaration.
Apply this same fix to all other methods in the file that have duplicate `@brief`
tags to ensure clean Doxygen output.
🪄 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: d13dd8c9-b3b5-4129-8a55-07149c99aa62
📒 Files selected for processing (13)
.github/workflows/doxygen.yml.gitignoreDoxyfileattacks.cppattacks.hfwd_decl.hmovegen.cppmoves_io.cppmoves_io.hposition.cppposition.hprinters.cpptypes.h
💤 Files with no reviewable changes (1)
- printers.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)
.github/workflows/doxygen.yml (1)
26-26:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpgrade
upload-artifactfrom@v4to a specific pinned version.Line 26 still uses
actions/upload-artifact@v4, which is unchanged from the past review and remains flagged as a supply-chain risk. Upgrade to@v6.0.3or pin to a full commit SHA for consistency with the checkout step and to address the prior review feedback.📋 Proposed fix
- name: Upload docs artifact - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@v6.0.3🤖 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/doxygen.yml at line 26, The actions/upload-artifact action in the doxygen.yml workflow is using the `@v4` tag which is not pinned to a specific version, creating a supply-chain risk. Update the uses directive for the upload-artifact action from `@v4` to `@v6.0.3` (or alternatively pin it to a full commit SHA) to match the security practice used in the checkout step and address the prior review feedback.
🤖 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/doxygen.yml:
- Around line 11-12: Standardize GitHub Actions version pinning across all
workflows by addressing three checkout actions and one upload-artifact action.
Add persist-credentials: false to the actions/checkout steps at
.github/workflows/doxygen.yml lines 11-12, .github/workflows/test.yml line 21,
and .github/workflows/test.yml line 55 to exclude unnecessary GitHub tokens.
Decide on a consistent version pinning policy (either immutable commit SHAs or
semantic version tags like v6.0.3) and apply it uniformly across all three
checkout steps; currently doxygen.yml uses v6.0.3, so ensure test.yml lines 21
and 55 match this same policy. Additionally, upgrade the upload-artifact action
at .github/workflows/doxygen.yml line 26 from `@v4` to `@v6.0.3` (or a full commit
SHA) to align with the chosen version strategy across the entire workflow suite.
---
Outside diff comments:
In @.github/workflows/doxygen.yml:
- Line 26: The actions/upload-artifact action in the doxygen.yml workflow is
using the `@v4` tag which is not pinned to a specific version, creating a
supply-chain risk. Update the uses directive for the upload-artifact action from
`@v4` to `@v6.0.3` (or alternatively pin it to a full commit SHA) to match the
security practice used in the checkout step and address the prior review
feedback.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: 43755f84-d9ed-4e30-b454-d9d3cf0800b8
📒 Files selected for processing (8)
.github/workflows/doxygen.yml.github/workflows/test.yml.gitignoreDoxyfilemovegen.cppposition.cppposition.htypes.h
🚧 Files skipped from review as they are similar to previous changes (5)
- .gitignore
- Doxyfile
- types.h
- movegen.cpp
- position.h
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The agent generated fixes only for
Lines 10–15 steps:
- name: Checkout
uses: actions/checkout@v6.0.3
+ with:
+ persist-credentials: false
- name: Install dependencies
run: |Lines 23–29 doxygen Doxyfile
- name: Upload docs artifact
- uses: actions/upload-artifact@v4
+ uses: actions/upload-artifact@v6.0.3
with:
name: doxygen-html
path: doc/html
Lines 21–26 - uses: actions/checkout@v6.0.3
with:
ref: ${{ github.head_ref }}
+ persist-credentials: false
- name: Install clang-format
run: sudo apt-get update && sudo apt-get install -y clang-formatLines 53–58
steps:
- uses: actions/checkout@v6.0.3
+ with:
+ persist-credentials: false
- name: Set build dir
id: vars |
Contributor
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @winapiadmin. The following files were modified: * `attacks.cpp` * `attacks.h` * `fwd_decl.h` * `movegen.cpp` * `moves_io.cpp` * `moves_io.h` * `position.cpp` * `position.h` * `printers.cpp` * `types.h` These file types are not supported: * `.github/workflows/doxygen.yml` * `.github/workflows/test.yml` * `.gitignore` * `Doxyfile`
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 (3)
position.cpp (3)
642-649:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate FEN counters before narrowing them.
halfMoveClockis stored asuint8_tandfullMoveNumberasuint16_t, but Line 643 only checks lower bounds. Values like halfmove256or fullmove70000silently wrap at Lines 646 and 649, corrupting FEN round-trips and draw detection.🛡️ Proposed fix
+#include <limits> ... // 5. Halfmove clock - INVALID_ARG_IF(halfmove < 0 || fullmove < 1, std::runtime_error("Invalid halfmove and/or fullmove counters")); - if (halfmove < 0 || fullmove < 1) + constexpr int max_halfmove = std::numeric_limits<uint8_t>::max(); + constexpr int max_fullmove = std::numeric_limits<uint16_t>::max(); + INVALID_ARG_IF(halfmove < 0 || halfmove > max_halfmove || fullmove < 1 || fullmove > max_fullmove, + std::runtime_error("Invalid halfmove and/or fullmove counters")); + if (halfmove < 0 || halfmove > max_halfmove || fullmove < 1 || fullmove > max_fullmove) return false; state().halfMoveClock = static_cast<uint8_t>(halfmove);🤖 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 642 - 649, The validation for halfmove and fullmove counters at line 642-643 only checks lower bounds but does not verify upper bounds before narrowing the values to uint8_t and uint16_t at lines 646 and 649. Add upper bound validation in the INVALID_ARG_IF condition to ensure halfmove fits within uint8_t range (maximum 255) and fullmove fits within uint16_t range (maximum 65535). This will prevent silent overflow and data corruption when out-of-range values are provided.
1047-1068:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard
SQ_NONEbefore shifting castling metadata.For positions with no castling rights,
castling_meta_[...].rook_start_*andking_startremainSQ_NONE.clean_castling_rights()still evaluates1ULL << SQ_NONE, which is undefined behavior whenis_valid()calls it.🛡️ Proposed fix
template <typename PieceC, typename T> CastlingRights _Position<PieceC, T>::clean_castling_rights() const { - const Bitboard cr_BOO = 1ULL << static_cast<int>(castling_meta_[BLACK].rook_start_ks); - const Bitboard cr_BOOO = 1ULL << static_cast<int>(castling_meta_[BLACK].rook_start_qs); - const Bitboard cr_WOO = 1ULL << static_cast<int>(castling_meta_[WHITE].rook_start_ks); - const Bitboard cr_WOOO = 1ULL << static_cast<int>(castling_meta_[WHITE].rook_start_qs); + const auto square_bb = [](Square sq) -> Bitboard { + return sq == SQ_NONE ? 0ULL : (1ULL << static_cast<int>(sq)); + }; + + const Bitboard cr_BOO = square_bb(castling_meta_[BLACK].rook_start_ks); + const Bitboard cr_BOOO = square_bb(castling_meta_[BLACK].rook_start_qs); + const Bitboard cr_WOO = square_bb(castling_meta_[WHITE].rook_start_ks); + const Bitboard cr_WOOO = square_bb(castling_meta_[WHITE].rook_start_qs); @@ - if (!(occ(WHITE) & pieces(KING) & (1ULL << castling_meta_[WHITE].king_start))) + const Bitboard white_king_start = square_bb(castling_meta_[WHITE].king_start); + const Bitboard black_king_start = square_bb(castling_meta_[BLACK].king_start); + if (!white_king_start || !(occ(WHITE) & pieces(KING) & white_king_start)) white_castling = 0; - if (!(occ(BLACK) & pieces(KING) & (1ULL << castling_meta_[BLACK].king_start))) + if (!black_king_start || !(occ(BLACK) & pieces(KING) & black_king_start)) black_castling = 0;🤖 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 1047 - 1068, The code in clean_castling_rights() shifts the `castling_meta_[...].rook_start_ks`, `castling_meta_[...].rook_start_qs`, and `castling_meta_[...].king_start` values without checking if they are SQ_NONE, causing undefined behavior when positions have no castling rights. Guard each of the four bitboard initializations (cr_BOO, cr_BOOO, cr_WOO, cr_WOOO) by verifying the corresponding square value is not SQ_NONE before performing the bit shift; similarly protect the king existence checks by verifying castling_meta_[WHITE].king_start and castling_meta_[BLACK].king_start are not SQ_NONE before creating their bitboard masks.
603-624:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBuild castling paths from side-specific rights and stored rook squares.
Line 605 and Line 615 mix
Color cwith castling-right masks, so white paths are skipped and black paths can be gated by the wrong rights bit. Line 607 and Line 617 also rebuild the rook square with swapped coordinate order; userook_start_ks/rook_start_qsdirectly. Downstream castling generation relies on these paths for occupancy checks.🐛 Proposed fix
for (Color c : { WHITE, BLACK }) { + const CastlingRights side_rights = c == WHITE ? WHITE_CASTLING : BLACK_CASTLING; // king - if (castlingRights() & (c & KING_SIDE)) { + if (castlingRights() & (side_rights & KING_SIDE)) { const auto king_from = castling_meta_[c].king_start; - const auto rook_from = make_sq(file_of(castling_meta_[c].rook_start_ks), rank_of(king_from)); + const auto rook_from = castling_meta_[c].rook_start_ks; const auto king_to = castling_king_square(c, true); const auto rook_to = castling_rook_square(c, true); castling_meta_[c].castling_paths[true] = (movegen::between(rook_from, rook_to) | movegen::between(king_from, king_to)) & ~((1ULL << king_from) | (1ULL << rook_from)); } // queen - if (castlingRights() & (c & QUEEN_SIDE)) { + if (castlingRights() & (side_rights & QUEEN_SIDE)) { const auto king_from = castling_meta_[c].king_start; - const auto rook_from = make_sq(file_of(castling_meta_[c].rook_start_qs), rank_of(king_from)); + const auto rook_from = castling_meta_[c].rook_start_qs; const auto king_to = castling_king_square(c, false); const auto rook_to = castling_rook_square(c, false); castling_meta_[c].castling_paths[false] = (movegen::between(rook_from, rook_to) | movegen::between(king_from, king_to)) & ~((1ULL << king_from) | (1ULL << rook_from));🤖 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 603 - 624, The castling path construction has two issues. First, the castling rights checks at lines 605 and 615 incorrectly mix the Color parameter c with KING_SIDE and QUEEN_SIDE masks using bitwise AND, causing white paths to be skipped and black paths to use wrong rights bits. Use the proper castling rights lookup for each color and side combination instead of mixing Color with mask values directly. Second, lines 607 and 617 unnecessarily reconstruct the rook square using make_sq with file_of and rank_of applied to castling_meta_[c].rook_start_ks and castling_meta_[c].rook_start_qs; instead, use these stored rook squares directly as-is without coordinate reconstruction. These fixes ensure castling paths are built correctly for both colors and sides, which downstream castling generation relies on for proper occupancy checks.
🤖 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 177-264: Delete the entire block containing the duplicate
MoveGenType enum definition and invalid namespace-scope member function
declarations (including the operator& template, operator| template, legals
function templates, doMove and do_move templates, undoMove template, and their
associated comments). These are placeholder duplicates that will cause
compilation errors since these should only be defined as class members within
the _Position class definition. Additionally, locate and remove the literal
KEEP_EXISTING placeholder text strings that appear elsewhere in the file
(referenced at lines 749 and 751) as they are syntactically invalid. The correct
and authoritative definitions of MoveGenType and the class member templates
already exist later in the file.
---
Outside diff comments:
In `@position.cpp`:
- Around line 642-649: The validation for halfmove and fullmove counters at line
642-643 only checks lower bounds but does not verify upper bounds before
narrowing the values to uint8_t and uint16_t at lines 646 and 649. Add upper
bound validation in the INVALID_ARG_IF condition to ensure halfmove fits within
uint8_t range (maximum 255) and fullmove fits within uint16_t range (maximum
65535). This will prevent silent overflow and data corruption when out-of-range
values are provided.
- Around line 1047-1068: The code in clean_castling_rights() shifts the
`castling_meta_[...].rook_start_ks`, `castling_meta_[...].rook_start_qs`, and
`castling_meta_[...].king_start` values without checking if they are SQ_NONE,
causing undefined behavior when positions have no castling rights. Guard each of
the four bitboard initializations (cr_BOO, cr_BOOO, cr_WOO, cr_WOOO) by
verifying the corresponding square value is not SQ_NONE before performing the
bit shift; similarly protect the king existence checks by verifying
castling_meta_[WHITE].king_start and castling_meta_[BLACK].king_start are not
SQ_NONE before creating their bitboard masks.
- Around line 603-624: The castling path construction has two issues. First, the
castling rights checks at lines 605 and 615 incorrectly mix the Color parameter
c with KING_SIDE and QUEEN_SIDE masks using bitwise AND, causing white paths to
be skipped and black paths to use wrong rights bits. Use the proper castling
rights lookup for each color and side combination instead of mixing Color with
mask values directly. Second, lines 607 and 617 unnecessarily reconstruct the
rook square using make_sq with file_of and rank_of applied to
castling_meta_[c].rook_start_ks and castling_meta_[c].rook_start_qs; instead,
use these stored rook squares directly as-is without coordinate reconstruction.
These fixes ensure castling paths are built correctly for both colors and sides,
which downstream castling generation relies on for proper occupancy checks.
🪄 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: cd18c899-634c-49f3-8a03-2d4ae4e9776e
📒 Files selected for processing (10)
attacks.cppattacks.hfwd_decl.hmovegen.cppmoves_io.cppmoves_io.hposition.cppposition.hprinters.cpptypes.h
✅ Files skipped from review due to trivial changes (3)
- fwd_decl.h
- moves_io.cpp
- moves_io.h
🚧 Files skipped from review as they are similar to previous changes (5)
- printers.cpp
- attacks.cpp
- types.h
- attacks.h
- movegen.cpp
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
CMakeLists.txt (1)
61-63: Consider using generator expressions forNDEBUGinstead ofCMAKE_BUILD_TYPEfor better multi-config generator support.The current check at Line 61 works in your CI environment because
-DCMAKE_BUILD_TYPEis explicitly set at configure time. However, if this project is ever built with a multi-config generator (Visual Studio, Xcode) without explicit-DCMAKE_BUILD_TYPE, the condition may fail silently andNDEBUGwon't be defined for release configurations. Using per-config generator expressions is a more robust approach:-if(CMAKE_BUILD_TYPE MATCHES "^(Release|RelWithDebInfo|MinSizeRel)$") - target_compile_definitions(chesslib INTERFACE NDEBUG) -endif() +target_compile_definitions(chesslib INTERFACE + "$<$<CONFIG:Release>:NDEBUG>" + "$<$<CONFIG:RelWithDebInfo>:NDEBUG>" + "$<$<CONFIG:MinSizeRel>:NDEBUG>" +)This ensures
NDEBUGis correctly defined regardless of the generator or how CMake is invoked.🤖 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 `@CMakeLists.txt` around lines 61 - 63, The current approach of checking CMAKE_BUILD_TYPE with a regex pattern for defining NDEBUG is not robust for multi-config generators like Visual Studio and Xcode. Replace the if() conditional block that checks CMAKE_BUILD_TYPE with a single target_compile_definitions call for the chesslib target that uses per-config generator expressions (using $<CONFIG:...> syntax) to conditionally define NDEBUG only for Release, RelWithDebInfo, and MinSizeRel configurations. This approach works consistently across all CMake generators regardless of how CMAKE_BUILD_TYPE is set or whether a multi-config generator is used.
🤖 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 `@CMakeLists.txt`:
- Around line 61-63: The current approach of checking CMAKE_BUILD_TYPE with a
regex pattern for defining NDEBUG is not robust for multi-config generators like
Visual Studio and Xcode. Replace the if() conditional block that checks
CMAKE_BUILD_TYPE with a single target_compile_definitions call for the chesslib
target that uses per-config generator expressions (using $<CONFIG:...> syntax)
to conditionally define NDEBUG only for Release, RelWithDebInfo, and MinSizeRel
configurations. This approach works consistently across all CMake generators
regardless of how CMAKE_BUILD_TYPE is set or whether a multi-config generator is
used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1fb3895c-c457-4e0a-b402-31b5c69449a5
📒 Files selected for processing (8)
.github/workflows/doxygen.yml.github/workflows/test.ymlCMakeLists.txtmovegen.cppmoves_io.cppposition.htests.cpptypes.h
✅ Files skipped from review due to trivial changes (2)
- moves_io.cpp
- tests.cpp
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/workflows/doxygen.yml
- .github/workflows/test.yml
- types.h
- movegen.cpp
- position.h
Merged
winapiadmin
added a commit
that referenced
this pull request
Jun 17, 2026
<optimizations branch #67 commits> * doxygen docgen bug (coderabbit artifact after failure) * compile bug * clang-format bug --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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.
No description provided.