Improve attack generation verification and Chess960 test structure#58
Improve attack generation verification and Chess960 test structure#58winapiadmin wants to merge 6 commits into
Conversation
…isable base nodes limit for debug mode, rank/file offsets
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR moves bishop/rook slider implementations out of the header, changes _POSSIBLY_CONSTEXPR so precomputed table linkage differs under GENERATE_AT_RUNTIME, adds Rank/File Direction operators, and introduces randomized attack verification plus a debug perft testcase splitter. ChangesAttack API and Coordinate Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 3f37b627ca
ℹ️ 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".
| std::cerr << "(skipped)\n"; | ||
| continue; | ||
| } | ||
| entries = split_testcases(entries); |
There was a problem hiding this comment.
In a default CMake test build (empty CMAKE_BUILD_TYPE, so NDEBUG is not defined and IS_RELEASE is 0), this replaces the old > 1e6 skip with split_testcases, which keeps 30 cases up to 10M nodes and five cases up to 1B nodes. With the current test data that is about 2.7B expected nodes per piece mapping before the loop repeats for the three piece representations, so test_normal can run for an impractical amount of time or time out instead of remaining a quick debug check; keep the cap or bound the sampled buckets to debug-sized cases.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
attacks.h (1)
326-335: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
slider<>()is markedconstexprbut unconditionally calls non-constexprfunctions.After this PR,
bishop()androok()are plain non-constexprdeclarations, andqueen()isinline(notconstexpr), yetslider<>()retains theconstexprqualifier at line 326. The function cannot be evaluated as a constant expression; theconstexprcontract is false. Change toinlineto reflect the actual behavior.Suggested change
-template <PieceType pt> [[nodiscard]] constexpr Bitboard slider(Square sq, Bitboard occupied) { +template <PieceType pt> [[nodiscard]] inline Bitboard slider(Square sq, Bitboard occupied) {🤖 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 `@attacks.h` around lines 326 - 335, The template function slider<PieceType> is declared constexpr but calls non-constexpr functions bishop(), rook(), and queen(), so change its declaration to inline (remove constexpr) to accurately reflect runtime evaluation; update the signature of slider (template <PieceType pt> inline Bitboard slider(Square sq, Bitboard occupied)) and keep the existing if constexpr branches that call bishop, rook, and queen.
🧹 Nitpick comments (4)
attacks.cpp (1)
93-186: 💤 Low valueEncapsulate runtime-generated magic data with internal linkage.
When
GENERATE_AT_RUNTIMEis defined,_POSSIBLY_CONSTEXPRcollapses to nothing, soRookMagics,BishopMagics,bishopData,BishopTable,BishopAttacks,rookData,RookTable, andRookAttacksall become mutable namespace-scope globals with external linkage. Since the header no longer exposes them (they’re strictly an implementation detail behindbishop()/rook()), wrapping them in an anonymous namespace (or marking themstatic) tightens the contract and prevents accidental external references at link time.🤖 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 `@attacks.cpp` around lines 93 - 186, The runtime-generated magic arrays and tables (RookMagics, BishopMagics, bishopData, BishopTable, BishopAttacks, rookData, RookTable, RookAttacks) currently have external linkage when GENERATE_AT_RUNTIME is defined; wrap these symbols in an anonymous namespace (or mark them static) so they get internal linkage, e.g., enclose the declarations/definitions of RookMagics, BishopMagics and the results from generate_magic_table() (bishopData/rookData and the derived BishopTable/BishopAttacks/RookTable/RookAttacks) in an anonymous namespace block to prevent accidental external linkage.tests.cpp (2)
388-404: ⚡ Quick winUse a deterministic seed so failures are reproducible.
std::random_devicemakes this test irreproducible across CI runs — when an assertion fails (e.g., on a regression inattacks::bishop/attacks::rook), the failing occupancy bitboard cannot be recovered to investigate. Seedmt19937_64with a fixed constant and (optionally) log the seed and currentbbon assertion to make triage tractable. Edge cases worth covering deterministically:occupied == 0,occupied == ~0ULL, and a single blocker on every ray.♻️ Suggested change
-TEST_CASE("attacks") { - std::random_device rd; - std::mt19937_64 gen(rd()); - std::uniform_int_distribution<uint64_t> dis; - for (Square sq = SQ_A1; sq < SQ_NONE; sq++) { - for (int i = 0; i < 10000; i++) { - uint64_t bb = dis(gen) & dis(gen); - REQUIRE(attacks::bishop(sq, bb) == sliderAttacks<false>(sq, bb)); - } - } - for (Square sq = SQ_A1; sq < SQ_NONE; sq++) { - for (int i = 0; i < 10000; i++) { - uint64_t bb = dis(gen) & dis(gen); - REQUIRE(attacks::rook(sq, bb) == sliderAttacks<true>(sq, bb)); - } - } -} +TEST_CASE("attacks") { + constexpr uint64_t kSeed = 0xC0DERABB17ULL; + std::mt19937_64 gen(kSeed); + std::uniform_int_distribution<uint64_t> dis; + for (Square sq = SQ_A1; sq < SQ_NONE; sq++) { + // deterministic edge cases first + REQUIRE(attacks::bishop(sq, 0ULL) == sliderAttacks<false>(sq, 0ULL)); + REQUIRE(attacks::bishop(sq, ~0ULL) == sliderAttacks<false>(sq, ~0ULL)); + REQUIRE(attacks::rook(sq, 0ULL) == sliderAttacks<true>(sq, 0ULL)); + REQUIRE(attacks::rook(sq, ~0ULL) == sliderAttacks<true>(sq, ~0ULL)); + for (int i = 0; i < 10000; i++) { + uint64_t bb = dis(gen) & dis(gen); + REQUIRE(attacks::bishop(sq, bb) == sliderAttacks<false>(sq, bb)); + REQUIRE(attacks::rook(sq, bb) == sliderAttacks<true>(sq, bb)); + } + } +}🤖 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 `@tests.cpp` around lines 388 - 404, Replace the non-deterministic random_device seed in the TEST_CASE("attacks") block with a fixed constant seed for std::mt19937_64 (e.g. a literal uint64_t) so tests are reproducible; additionally add deterministic checks for occupied==0, occupied==~0ULL and a loop that constructs a single-blocker occupancy on every ray for each Square to compare attacks::bishop/attacks::rook with sliderAttacks, and when calling REQUIRE include/log the seed and the failing bb (occupancy) so failures can be reproduced and triaged; update references in this test to use the fixed seed generator (gen) and ensure both bishop and rook loops use the same deterministic inputs.
264-272: 💤 Low valueEntries with > 1,000,000,000 nodes are silently dropped.
The bucket cascade ends at
<= 1'000'000'000, so any future test case exceeding 1B nodes will be filtered out without warning rather than landing in a "huge" bucket. Today no entry breaches that threshold (max in the suite is ~525M), but it's a quiet footgun for future contributors. Either widen the last bucket to a sentinel (e.g., omit the upper bound) or add a comment documenting the deliberate cap.♻️ Suggested change
- else if (e.info.nodes <= 1'000'000'000) - bucket3.push_back(e); + else + bucket3.push_back(e); // anything above 10M lands here, including very heavy positions🤖 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 `@tests.cpp` around lines 264 - 272, The current cascade over entries silently drops any TestEntry whose e.info.nodes > 1'000'000'000 because the last condition is <= 1'000'000'000; change the final branch to a no-upper-bound catch-all (replace "else if (e.info.nodes <= 1'000'000'000) bucket3.push_back(e);" with a plain "else bucket3.push_back(e);") or explicitly add a fourth "huge" bucket and push those entries there, and add a brief comment near bucket3/bucket4 explaining the chosen sentinel handling so future tests >1B nodes are not lost.types.h (1)
193-200: 💤 Low valueDirection values applied to
Rank/Filecan yield out-of-domain results.
Directionis a square-step encoding (NORTH = 8,SOUTH = -8, etc.), not an axis-step. AddingNORTHto aRank(orFile) producesRank(r + 8), which is nonsensical and silently leaves the [0,7] domain. The only current caller intests.cpp::sliderAttackshappens to pass onlyEAST/WEST/DIR_NONE, whose numeric values (±1,0) coincidentally work as axis offsets, but the type-level contract is misleading and easy to misuse going forward.Consider asserting validity (or constraining with a separate axis-offset type) so that misuse is caught instead of silently propagating invalid values.
🛡️ Optional: assert valid 1-step axis usage
-constexpr Rank operator+(Rank s, Direction d) { return Rank(int(s) + int(d)); } -constexpr Rank operator-(Rank s, Direction d) { return Rank(int(s) - int(d)); } +constexpr Rank operator+(Rank s, Direction d) { + assert(d == EAST || d == WEST || d == DIR_NONE); + return Rank(int(s) + int(d)); +} +constexpr Rank operator-(Rank s, Direction d) { + assert(d == EAST || d == WEST || d == DIR_NONE); + return Rank(int(s) - int(d)); +}(Same for
Fileand the compound forms.)🤖 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 193 - 200, The Rank/File arithmetic operators (constexpr operator+, operator-, operator+=, operator-= for Rank and File) accept Direction which is a square-step (e.g. NORTH=8) and can produce out-of-domain values; change these operators so they either assert the Direction is a 1-step axis offset (check that int(d) is -1,0,1) or replace the parameter type with a dedicated AxisOffset/AxisStep type (e.g. AxisOffset) and update callers (tests.cpp::sliderAttacks) accordingly; ensure all four operator overloads for Rank and File perform the validation or use the new type so invalid additions like +NORTH/+SOUTH are caught rather than silently creating invalid Rank/File values.
🤖 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 `@attacks.h`:
- Around line 326-335: The template function slider<PieceType> is declared
constexpr but calls non-constexpr functions bishop(), rook(), and queen(), so
change its declaration to inline (remove constexpr) to accurately reflect
runtime evaluation; update the signature of slider (template <PieceType pt>
inline Bitboard slider(Square sq, Bitboard occupied)) and keep the existing if
constexpr branches that call bishop, rook, and queen.
---
Nitpick comments:
In `@attacks.cpp`:
- Around line 93-186: The runtime-generated magic arrays and tables (RookMagics,
BishopMagics, bishopData, BishopTable, BishopAttacks, rookData, RookTable,
RookAttacks) currently have external linkage when GENERATE_AT_RUNTIME is
defined; wrap these symbols in an anonymous namespace (or mark them static) so
they get internal linkage, e.g., enclose the declarations/definitions of
RookMagics, BishopMagics and the results from generate_magic_table()
(bishopData/rookData and the derived
BishopTable/BishopAttacks/RookTable/RookAttacks) in an anonymous namespace block
to prevent accidental external linkage.
In `@tests.cpp`:
- Around line 388-404: Replace the non-deterministic random_device seed in the
TEST_CASE("attacks") block with a fixed constant seed for std::mt19937_64 (e.g.
a literal uint64_t) so tests are reproducible; additionally add deterministic
checks for occupied==0, occupied==~0ULL and a loop that constructs a
single-blocker occupancy on every ray for each Square to compare
attacks::bishop/attacks::rook with sliderAttacks, and when calling REQUIRE
include/log the seed and the failing bb (occupancy) so failures can be
reproduced and triaged; update references in this test to use the fixed seed
generator (gen) and ensure both bishop and rook loops use the same deterministic
inputs.
- Around line 264-272: The current cascade over entries silently drops any
TestEntry whose e.info.nodes > 1'000'000'000 because the last condition is <=
1'000'000'000; change the final branch to a no-upper-bound catch-all (replace
"else if (e.info.nodes <= 1'000'000'000) bucket3.push_back(e);" with a plain
"else bucket3.push_back(e);") or explicitly add a fourth "huge" bucket and push
those entries there, and add a brief comment near bucket3/bucket4 explaining the
chosen sentinel handling so future tests >1B nodes are not lost.
In `@types.h`:
- Around line 193-200: The Rank/File arithmetic operators (constexpr operator+,
operator-, operator+=, operator-= for Rank and File) accept Direction which is a
square-step (e.g. NORTH=8) and can produce out-of-domain values; change these
operators so they either assert the Direction is a 1-step axis offset (check
that int(d) is -1,0,1) or replace the parameter type with a dedicated
AxisOffset/AxisStep type (e.g. AxisOffset) and update callers
(tests.cpp::sliderAttacks) accordingly; ensure all four operator overloads for
Rank and File perform the validation or use the new type so invalid additions
like +NORTH/+SOUTH are caught rather than silently creating invalid Rank/File
values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b749e216-230a-4da0-b92a-d6f7cbdab810
📒 Files selected for processing (5)
attacks.cppattacks.hchess960_tests.cpptests.cpptypes.h
💤 Files with no reviewable changes (1)
- chess960_tests.cpp
Reduce the number of elements taken from bucket3 from 5 to 1.
Reduce the limits for bucket2 and bucket3 sizes in optimization.
Replaced random device initialization with a fixed seed for reproducibility in tests.
1 participant
No description provided.