diff --git a/library/src/heuristic_sorting/heuristic_sorting.cpp b/library/src/heuristic_sorting/heuristic_sorting.cpp index d175e451..1270fa59 100644 --- a/library/src/heuristic_sorting/heuristic_sorting.cpp +++ b/library/src/heuristic_sorting/heuristic_sorting.cpp @@ -2,66 +2,31 @@ #include #include -// New overload: accepts a pre-built HeuristicContext. This contains the -// same inline logic that used to be in the previous function body. -void call_heuristic(const HeuristicContext& context) +// The caller passes the weight case it already knows, so nothing is +// re-derived from the context here. +void call_heuristic(HeuristicContext& context, const WeightCase weight_case) { - // Determine which position in trick (0=leading, 1-3=following) - int hand_rel = 0; - if (context.curr_hand != context.lead_hand) { - // Calculate relative position: 1, 2, or 3 based on lead hand - hand_rel = (context.curr_hand + 4 - context.lead_hand) % 4; - } - - // Leading hand (hand_rel == 0) - MoveGen0 logic - if (hand_rel == 0) { - // Check if trump game with trump winner available - bool trump_game = (context.trump != DDS_NOTRUMP) && - (context.trump >= 0 && context.trump < DDS_SUITS) && - (context.tpos.winner[context.trump].rank != 0); - - if (trump_game) { - weight_alloc_trump0(const_cast(context)); - } else { - weight_alloc_nt0(const_cast(context)); - } - return; - } - - // Following hands (hand_rel 1-3) - MoveGen123 logic - // Check trump game condition - int ftest = ((context.trump != DDS_NOTRUMP) && - (context.trump >= 0 && context.trump < DDS_SUITS) && - (context.tpos.winner[context.trump].rank != 0) ? 1 : 0); - - // Check if current hand can follow suit (not void) - unsigned short ris = context.tpos.rank_in_suit[context.curr_hand][context.lead_suit]; - bool can_follow_suit = (ris != 0); - - // Calculate function index using same logic as original - int findex; - if (can_follow_suit) { - findex = 4 * hand_rel + ftest; - } else { - findex = 4 * hand_rel + ftest + 2; - } - - // Following hands function dispatch table (MoveGen123 logic) - switch (findex) { - case 4: weight_alloc_nt_notvoid1(const_cast(context)); break; // hand_rel=1, can follow, no trump - case 5: weight_alloc_trump_notvoid1(const_cast(context)); break; // hand_rel=1, can follow, trump - case 6: weight_alloc_nt_void1(const_cast(context)); break; // hand_rel=1, void, no trump - case 7: weight_alloc_trump_void1(const_cast(context)); break; // hand_rel=1, void, trump - case 8: weight_alloc_nt_notvoid2(const_cast(context)); break; // hand_rel=2, can follow, no trump - case 9: weight_alloc_trump_notvoid2(const_cast(context)); break; // hand_rel=2, can follow, trump - case 10: weight_alloc_nt_void2(const_cast(context)); break; // hand_rel=2, void, no trump - case 11: weight_alloc_trump_void2(const_cast(context)); break; // hand_rel=2, void, trump - case 12: weight_alloc_combined_notvoid3(const_cast(context)); break; // hand_rel=3, can follow, no trump - case 13: weight_alloc_combined_notvoid3(const_cast(context)); break; // hand_rel=3, can follow, trump - case 14: weight_alloc_nt_void3(const_cast(context)); break; // hand_rel=3, void, no trump - case 15: weight_alloc_trump_void3(const_cast(context)); break; // hand_rel=3, void, trump - default: - // Should not happen, but default to basic sorting + switch (weight_case) { + case WeightCase::Nt0: weight_alloc_nt0(context); break; + case WeightCase::Trump0: weight_alloc_trump0(context); break; + case WeightCase::NtNotVoid1: weight_alloc_nt_notvoid1(context); break; + case WeightCase::TrumpNotVoid1: weight_alloc_trump_notvoid1(context); break; + case WeightCase::NtVoid1: weight_alloc_nt_void1(context); break; + case WeightCase::TrumpVoid1: weight_alloc_trump_void1(context); break; + case WeightCase::NtNotVoid2: weight_alloc_nt_notvoid2(context); break; + case WeightCase::TrumpNotVoid2: weight_alloc_trump_notvoid2(context); break; + case WeightCase::NtVoid2: weight_alloc_nt_void2(context); break; + case WeightCase::TrumpVoid2: weight_alloc_trump_void2(context); break; + case WeightCase::CombinedNotVoid3: + case WeightCase::CombinedNotVoid3Trump: + weight_alloc_combined_notvoid3(context); + break; + case WeightCase::NtVoid3: weight_alloc_nt_void3(context); break; + case WeightCase::TrumpVoid3: weight_alloc_trump_void3(context); break; + default: + // Should not happen; fall back to NT leading weights so moves get + // deterministic ordering instead of stale/uninitialized weights. + weight_alloc_nt0(context); break; } } diff --git a/library/src/heuristic_sorting/heuristic_sorting.hpp b/library/src/heuristic_sorting/heuristic_sorting.hpp index 83ed14df..ced35abf 100644 --- a/library/src/heuristic_sorting/heuristic_sorting.hpp +++ b/library/src/heuristic_sorting/heuristic_sorting.hpp @@ -35,7 +35,9 @@ struct HeuristicContext int num_moves; int last_num_moves; const int trump; - const int suit; // For MoveGen0, the suit being considered + // For MoveGen0, the suit being considered. Mutable so callers can build + // the context once per move generation and update it per suit iteration. + int suit; const TrackType* trackp; const int curr_trick; const int curr_hand; @@ -60,18 +62,40 @@ struct HeuristicContext int lead0_rank = 0; // trackp->move[0].rank }; -/// @brief Apply heuristic sorting to candidate moves in the given context. +/// @brief Which weight_alloc_* helper to run for the current move list. +/// +/// Move generation already knows the lead/follow, trump-winner, and void +/// situation; it passes the matching case so call_heuristic does not re-derive +/// it from the context. Numeric values match the historical findex encoding +/// used by Moves::MoveGen0 / MoveGen123 (and DDS_MOVES RegisterList). +enum class WeightCase : int { + Nt0 = 0, ///< Leading hand, no trump winner available + Trump0 = 1, ///< Leading hand, trump winner available + NtNotVoid1 = 4, ///< 2nd hand, can follow, no trump winner + TrumpNotVoid1 = 5, ///< 2nd hand, can follow, trump winner + NtVoid1 = 6, ///< 2nd hand, void in lead suit, no trump winner + TrumpVoid1 = 7, ///< 2nd hand, void in lead suit, trump winner + NtNotVoid2 = 8, ///< 3rd hand, can follow, no trump winner + TrumpNotVoid2 = 9, ///< 3rd hand, can follow, trump winner + NtVoid2 = 10, ///< 3rd hand, void in lead suit, no trump winner + TrumpVoid2 = 11, ///< 3rd hand, void in lead suit, trump winner + CombinedNotVoid3 = 12, ///< 4th hand, can follow, no trump winner + CombinedNotVoid3Trump = 13,///< 4th hand, can follow, trump winner + NtVoid3 = 14, ///< 4th hand, void in lead suit, no trump winner + TrumpVoid3 = 15, ///< 4th hand, void in lead suit, trump winner +}; + +/// @brief Apply heuristic sorting using a precomputed weight case. /// /// Evaluates candidate moves using position-dependent heuristics to assign /// weights that guide search algorithms toward the most promising lines. -/// Heuristics vary based on position characteristics (leading/following hand, -/// trump presence, suit availability) to optimize move ordering efficiency. +/// Move generation already knows the dispatch case; passing it avoids +/// re-deriving the relative hand, trump state and voidness from the context +/// on every call. /// /// @param context Pre-constructed HeuristicContext containing position data, /// move arrays, and cached snapshots for efficient evaluation. -/// -/// @note This function mutates the context's move weighting arrays. -/// @note Prefer this overload over parameterized versions to minimize -/// construction overhead in hot paths. -void call_heuristic(const HeuristicContext& context); +/// Non-const because weighting writes through context.mply. +/// @param weight_case Precomputed weight case (see WeightCase). +void call_heuristic(HeuristicContext& context, WeightCase weight_case); diff --git a/library/src/moves/moves.cpp b/library/src/moves/moves.cpp index 00a42101..e2d5d939 100644 --- a/library/src/moves/moves.cpp +++ b/library/src/moves/moves.cpp @@ -49,11 +49,45 @@ const MgType RegisterList[16] = {MgType::NT0, MgType::TRUMP0, #define MG_REGISTER(a, b) 1; #endif +namespace +{ + +// Trump-winner bit of WeightCase: never index winner[trump] +// unless trump is a suit in [0, DDS_SUITS). +auto trump_winner_bit(const Pos& tpos, const int trump) -> int +{ + return ((trump != DDS_NOTRUMP) && + (trump >= 0 && trump < DDS_SUITS) && + (tpos.winner[trump].rank != 0)) + ? 1 + : 0; +} + +// Encode following-hand WeightCase: 4 * hand_rel + trump_winner + (void ? 2 : 0). +auto weight_case_follow(const int hand_rel, const int trump_winner, + const bool is_void) -> WeightCase +{ + return static_cast( + 4 * hand_rel + trump_winner + (is_void ? 2 : 0)); +} + +} // namespace + Moves::Moves() { // Initialize non-owning pointers to nullptr for safety trackp = nullptr; mply = nullptr; + // Scalars snapshotted by make_heuristic_context must not be indeterminate. + leadHand = 0; + currHand = 0; + leadSuit = 0; + currTrick = 0; + trump = DDS_NOTRUMP; + suit = 0; + numMoves = 0; + lastNumMoves = 0; + funcName[static_cast(MgType::NT0)] = "NT0"; funcName[static_cast(MgType::TRUMP0)] = "Trump0"; funcName[static_cast(MgType::NT_VOID1)] = "NT_Void1"; @@ -171,7 +205,21 @@ auto Moves::MoveGen0(const int tricks, const Pos &tpos, mply = list.move; for (int s = 0; s < DDS_SUITS; s++) trackp->lowest_win[0][s] = 0; + // Reset fields snapshotted by make_heuristic_context before the hoist; + // suit/lastNumMoves are overwritten again before each call_heuristic. numMoves = 0; + lastNumMoves = 0; + suit = 0; + leadSuit = 0; + + // Leading-hand weight case, known here once instead of re-derived per + // suit inside the heuristic dispatcher. + const WeightCase lead_case = trump_winner_bit(tpos, trump) + ? WeightCase::Trump0 + : WeightCase::Nt0; + HeuristicContext hctx = + make_heuristic_context(tpos, bestMove, bestMoveTT, thrp_rel, + track[tricks]); for (suit = 0; suit < DDS_SUITS; suit++) { unsigned short ris = tpos.rank_in_suit[leadHand][suit]; @@ -202,12 +250,14 @@ auto Moves::MoveGen0(const int tricks, const Pos &tpos, g--; } - Moves::call_heuristic(tpos, bestMove, bestMoveTT, thrp_rel); + hctx.suit = suit; + hctx.last_num_moves = lastNumMoves; + hctx.num_moves = numMoves; + ::call_heuristic(hctx, lead_case); } #ifdef DDS_MOVES - bool ftest = ((trump != DDS_NOTRUMP) && (tpos.winner[trump].rank != 0)); - if (ftest) + if (lead_case == WeightCase::Trump0) MG_REGISTER(MgType::TRUMP0, 0); else MG_REGISTER(MgType::NT0, 0); @@ -237,11 +287,19 @@ auto Moves::MoveGen123(const int tricks, const int handRel, const Pos &tpos) for (int s = 0; s < DDS_SUITS; s++) trackp->lowest_win[handRel][s] = 0; + // Reset fields snapshotted by make_heuristic_context before either hoist. + // Follow-suit uses suit == leadSuit; the void path overwrites suit per + // iteration before call_heuristic. numMoves = 0; + lastNumMoves = 0; + suit = leadSuit; + + WeightCase weight_case; + const int trump_winner = trump_winner_bit(tpos, trump); - [[maybe_unused]] int findex; - int ftest = - ((trump != DDS_NOTRUMP) && (tpos.winner[trump].rank != 0) ? 1 : 0); + // Empty best-move placeholders must outlive the hoisted context, which + // holds references to them. + const MoveType empty_move{}; unsigned short ris = tpos.rank_in_suit[currHand][leadSuit]; @@ -265,27 +323,35 @@ auto Moves::MoveGen123(const int tricks, const int handRel, const Pos &tpos) g--; } - findex = 4 * handRel + ftest; + weight_case = weight_case_follow(handRel, trump_winner, /*is_void=*/false); #ifdef DDS_MOVES - MG_REGISTER(RegisterList[findex], handRel); + MG_REGISTER(RegisterList[static_cast(weight_case)], handRel); #endif list.current = 0; list.last = numMoves - 1; if (numMoves == 1) return numMoves; - Moves::call_heuristic(tpos, MoveType{}, MoveType{}, nullptr); + + HeuristicContext hctx = + make_heuristic_context(tpos, empty_move, empty_move, nullptr, + track[tricks]); + ::call_heuristic(hctx, weight_case); Moves::MergeSort(); return numMoves; } - findex = 4 * handRel + ftest + 2; + weight_case = weight_case_follow(handRel, trump_winner, /*is_void=*/true); #ifdef DDS_MOVES - MG_REGISTER(RegisterList[findex], handRel); + MG_REGISTER(RegisterList[static_cast(weight_case)], handRel); #endif + HeuristicContext hctx = + make_heuristic_context(tpos, empty_move, empty_move, nullptr, + track[tricks]); + for (suit = 0; suit < DDS_SUITS; suit++) { ris = tpos.rank_in_suit[currHand][suit]; if (ris == 0) @@ -311,7 +377,10 @@ auto Moves::MoveGen123(const int tricks, const int handRel, const Pos &tpos) g--; } - Moves::call_heuristic(tpos, MoveType{}, MoveType{}, nullptr); + hctx.suit = suit; + hctx.last_num_moves = lastNumMoves; + hctx.num_moves = numMoves; + ::call_heuristic(hctx, weight_case); } list.current = 0; @@ -639,37 +708,38 @@ auto Moves::Sort(const int tricks, const int relHand) -> void { mply[j] = tmp; \ } -/** - * @brief Build a heuristic context and invoke heuristic sorting. - * - * Centralizes the construction of HeuristicContext to keep the call sites - * consistent and avoid exposing mutable state to heuristic helpers. - */ -auto Moves::call_heuristic(const Pos &tpos, const MoveType &best_move, - const MoveType &best_move_tt, - const RelRanksType thrp_rel[]) -> void { - // Construct context once here and call the context-taking overload. +auto Moves::make_heuristic_context(const Pos &tpos, const MoveType &best_move, + const MoveType &best_move_tt, + const RelRanksType thrp_rel[], + const TrackType &tr) const + -> HeuristicContext { HeuristicContext context{ tpos, best_move, best_move_tt, thrp_rel, mply, numMoves, lastNumMoves, - trump, suit, trackp, currTrick, currHand, leadHand, leadSuit}; - - // Snapshot removed_ranks into the context to avoid direct dependence on - // the mutable Moves::trackp buffer inside heuristic code. - for (int s = 0; s < DDS_SUITS; ++s) { - context.removed_ranks[s] = trackp ? trackp->removed_ranks[s] : 0; + trump, suit, &tr, currTrick, currHand, leadHand, leadSuit}; + + // Snapshot removed_ranks and only those trick-card fields that are defined + // for the current relative hand. Earlier slots may be unpopulated (MoveGen0 + // / hand_rel 1–2), so leave the rest at HeuristicContext's zero defaults. + for (int s = 0; s < DDS_SUITS; ++s) + context.removed_ranks[s] = tr.removed_ranks[s]; + + const int hand_rel = + (currHand == leadHand) ? 0 : (currHand + 4 - leadHand) % 4; + if (hand_rel >= 1) + context.lead0_rank = tr.move[0].rank; + if (hand_rel >= 2) + { + context.move1_rank = tr.move[1].rank; + context.move1_suit = tr.move[1].suit; + context.high1 = tr.high[1]; + } + if (hand_rel >= 3) + { + context.move2_rank = tr.move[2].rank; + context.move2_suit = tr.move[2].suit; + context.high2 = tr.high[2]; } - // Snapshot minimal trick state for helper usage. - context.move1_rank = (trackp ? trackp->move[1].rank : 0); - context.high1 = (trackp ? trackp->high[1] : 0); - context.move1_suit = (trackp ? trackp->move[1].suit : 0); - // Third-hand snapshots - context.move2_rank = (trackp ? trackp->move[2].rank : 0); - context.move2_suit = (trackp ? trackp->move[2].suit : 0); - context.high2 = (trackp ? trackp->high[2] : 0); - // Leader snapshot - context.lead0_rank = (trackp ? trackp->move[0].rank : 0); - - ::call_heuristic(context); + return context; } /** diff --git a/library/src/moves/moves.hpp b/library/src/moves/moves.hpp index 97dd55e0..99842b62 100644 --- a/library/src/moves/moves.hpp +++ b/library/src/moves/moves.hpp @@ -184,16 +184,23 @@ class Moves { auto MergeSort() -> void; /** - * @brief Invoke heuristic sorting for current move list. + * @brief Build a HeuristicContext snapshot from current move-gen state. + * + * Built once per move-generation call; the caller updates suit and move + * counts per suit iteration and dispatches via call_heuristic(ctx, weight_case). * * @param tpos Current position - * @param best_move Best move from search - * @param best_move_tt Best move from transposition table + * @param best_move Best move from search (must outlive the context) + * @param best_move_tt Best move from transposition table (must outlive the context) * @param thrp_rel Relative ranks per hand + * @param tr Bound trick track (must outlive the context). Passed explicitly + * so callers cannot accidentally dereference a null Moves::trackp. */ - auto call_heuristic(const Pos &tpos, const MoveType &best_move, - const MoveType &best_move_tt, const RelRanksType thrp_rel[]) - -> void; + auto make_heuristic_context(const Pos &tpos, const MoveType &best_move, + const MoveType &best_move_tt, + const RelRanksType thrp_rel[], + const TrackType &tr) const + -> HeuristicContext; // (logging accessors removed) diff --git a/library/tests/heuristic_sorting/BUILD.bazel b/library/tests/heuristic_sorting/BUILD.bazel index 3f564dd7..695a1bb3 100644 --- a/library/tests/heuristic_sorting/BUILD.bazel +++ b/library/tests/heuristic_sorting/BUILD.bazel @@ -14,6 +14,17 @@ cc_test( ], ) +cc_test( + name = "dispatch_findex_test", + size = "small", + srcs = ["dispatch_findex_test.cpp"], + deps = [ + "//library/src/api:api_definitions", + "//library/src/heuristic_sorting:testable_heuristic_sorting", + "@googletest//:gtest_main", + ], +) + cc_test( name = "merge_scratch_test", size = "small", diff --git a/library/tests/heuristic_sorting/dispatch_findex_test.cpp b/library/tests/heuristic_sorting/dispatch_findex_test.cpp new file mode 100644 index 00000000..94bebc01 --- /dev/null +++ b/library/tests/heuristic_sorting/dispatch_findex_test.cpp @@ -0,0 +1,160 @@ +/** + * @file dispatch_findex_test.cpp + * @brief Behaviour of WeightCase-based heuristic dispatch. + * + * Move generation always passes a precomputed WeightCase. This file covers the + * dispatcher's fallback when an unrecognized case reaches call_heuristic. + */ + +#include + +#include "heuristic_sorting/heuristic_sorting.hpp" +#include "heuristic_sorting/internal.hpp" +#include + +namespace +{ + +constexpr int kNumMoves = 4; +constexpr int kNumRelRanks = 8192; +static RelRanksType rel_ranks[kNumRelRanks] = {}; + +struct DispatchFixture +{ + Pos tpos{}; + MoveType best_move{}; + MoveType best_move_tt{}; + RelRanksType* rel = rel_ranks; + TrackType track{}; + MoveType moves_expected[kNumMoves]{}; + MoveType moves_dispatched[kNumMoves]{}; +}; + +void fill_position(DispatchFixture& f, const int curr_hand, const int lead_suit) +{ + for (int h = 0; h < DDS_HANDS; h++) + { + for (int s = 0; s < DDS_SUITS; s++) + { + f.tpos.rank_in_suit[h][s] = + static_cast(0x0155u << ((h + s) % 3)); + f.tpos.length[h][s] = static_cast(3 + ((h + s) % 3)); + } + f.tpos.hand_dist[h] = 13; + } + for (int s = 0; s < DDS_SUITS; s++) + { + f.tpos.aggr[s] = 0x1FF5; + f.tpos.winner[s].rank = 14; + f.tpos.winner[s].hand = (s + 1) % DDS_HANDS; + f.tpos.second_best[s].rank = 13; + f.tpos.second_best[s].hand = (s + 2) % DDS_HANDS; + } + + f.track.lead_suit = lead_suit; + f.track.move[0] = ExtCard{lead_suit, 8, 0}; + f.track.move[1] = ExtCard{lead_suit, 10, 0}; + f.track.move[2] = ExtCard{lead_suit, 6, 0}; + f.track.high[1] = 1; + f.track.high[2] = 1; + for (int s = 0; s < DDS_SUITS; s++) + f.track.removed_ranks[s] = 0x0022; + + for (int k = 0; k < kNumMoves; k++) + { + f.moves_expected[k] = MoveType{lead_suit, 12 - 2 * k, 0, 0}; + f.moves_dispatched[k] = f.moves_expected[k]; + } +} + +HeuristicContext make_context(DispatchFixture& f, MoveType* mply, + const int curr_hand, const int lead_hand, + const int lead_suit) +{ + HeuristicContext ctx = { + f.tpos, + f.best_move, + f.best_move_tt, + f.rel, + mply, + kNumMoves, // num_moves + 0, // last_num_moves + DDS_NOTRUMP, + lead_suit, // suit under consideration + &f.track, + 5, // curr_trick + curr_hand, + lead_hand, + lead_suit, + }; + for (int s = 0; s < DDS_SUITS; s++) + ctx.removed_ranks[s] = f.track.removed_ranks[s]; + ctx.move1_rank = f.track.move[1].rank; + ctx.high1 = f.track.high[1]; + ctx.move1_suit = f.track.move[1].suit; + ctx.move2_rank = f.track.move[2].rank; + ctx.move2_suit = f.track.move[2].suit; + ctx.high2 = f.track.high[2]; + ctx.lead0_rank = f.track.move[0].rank; + return ctx; +} + +} // namespace + +// Unrecognized WeightCase values must still assign deterministic weights +// (Nt0 fallback), not leave stale/uninitialized move weights that would +// scramble MergeSort. +TEST(DispatchWeightCase, InvalidWeightCaseFallsBackToBasicNt0Weights) +{ + for (const int bad_value : {2, 3, 16, -1, 99}) + { + DispatchFixture f; + const int lead_hand = 0; + fill_position(f, lead_hand, /*lead_suit=*/0); + + constexpr int kStale = 0x7f0f0f0f; + for (int k = 0; k < kNumMoves; k++) + { + f.moves_expected[k].weight = 0; + f.moves_dispatched[k].weight = kStale; + } + + HeuristicContext expected = make_context( + f, f.moves_expected, lead_hand, lead_hand, 0); + HeuristicContext dispatched = make_context( + f, f.moves_dispatched, lead_hand, lead_hand, 0); + + weight_alloc_nt0(expected); + call_heuristic(dispatched, static_cast(bad_value)); + + for (int k = 0; k < kNumMoves; k++) + { + EXPECT_NE(f.moves_dispatched[k].weight, kStale) + << "weight_case=" << bad_value << " move=" << k + << " left a stale weight"; + EXPECT_EQ(f.moves_expected[k].weight, f.moves_dispatched[k].weight) + << "weight_case=" << bad_value << " move=" << k + << " must match weight_alloc_nt0 fallback"; + } + } +} + +// Known WeightCase values must select the matching weight_alloc_* helper. +TEST(DispatchWeightCase, Nt0DispatchesToWeightAllocNt0) +{ + DispatchFixture f; + const int lead_hand = 0; + fill_position(f, lead_hand, /*lead_suit=*/0); + + HeuristicContext expected = make_context( + f, f.moves_expected, lead_hand, lead_hand, 0); + HeuristicContext dispatched = make_context( + f, f.moves_dispatched, lead_hand, lead_hand, 0); + + weight_alloc_nt0(expected); + call_heuristic(dispatched, WeightCase::Nt0); + + for (int k = 0; k < kNumMoves; k++) + EXPECT_EQ(f.moves_expected[k].weight, f.moves_dispatched[k].weight) + << "move=" << k; +} diff --git a/library/tests/moves/moves_test.cpp b/library/tests/moves/moves_test.cpp index f36b97cc..f01e2dcf 100644 --- a/library/tests/moves/moves_test.cpp +++ b/library/tests/moves/moves_test.cpp @@ -67,6 +67,17 @@ TEST_F(MovesTest, ConstructorInitializesState) { // Verify statistics are zeroed EXPECT_EQ(moves->trickFuncTable.nfuncs, 0); EXPECT_EQ(moves->trickFuncSuitTable.nfuncs, 0); + + // make_heuristic_context snapshots these; they must not be indeterminate + // after construction (and MoveGen0/123 re-set them before hoisting a context). + EXPECT_EQ(moves->leadHand, 0); + EXPECT_EQ(moves->currHand, 0); + EXPECT_EQ(moves->leadSuit, 0); + EXPECT_EQ(moves->currTrick, 0); + EXPECT_EQ(moves->trump, DDS_NOTRUMP); + EXPECT_EQ(moves->suit, 0); + EXPECT_EQ(moves->numMoves, 0); + EXPECT_EQ(moves->lastNumMoves, 0); } TEST_F(MovesTest, InitializesTrackingState) { @@ -262,6 +273,282 @@ TEST_F(MovesTest, MemorySafetyFeaturesArePresent) { EXPECT_FALSE(moves->funcName[0].empty()); // funcName array exists and is initialized } +namespace +{ + +auto poisoned_track() -> TrackType +{ + TrackType tr{}; + for (int s = 0; s < DDS_SUITS; ++s) + tr.removed_ranks[s] = 0x10 + s; + // Non-zero trick slots: only those defined for the current hand_rel should + // appear in the HeuristicContext snapshot. + tr.move[0] = ExtCard{0, 14, 0}; + tr.move[1] = ExtCard{2, 12, 0}; + tr.high[1] = 1; + tr.move[2] = ExtCard{3, 9, 0}; + tr.high[2] = 2; + return tr; +} + +// HeuristicContext stores references to these; they must outlive the returned +// value (locals in context_for_hand_rel would dangle after return). +static const Pos kTpos{}; +static const MoveType kBest{}; +static const MoveType kBestTt{}; + +auto context_for_hand_rel(Moves& m, const TrackType& tr, const int hand_rel) + -> HeuristicContext +{ + m.leadHand = 0; + m.currHand = hand_rel; // leadHand 0 ⇒ hand_rel == currHand + m.leadSuit = 0; + m.currTrick = 0; + m.trump = DDS_NOTRUMP; + m.suit = 0; + m.numMoves = 0; + m.lastNumMoves = 0; + return m.make_heuristic_context(kTpos, kBest, kBestTt, nullptr, tr); +} + +} // namespace + +/** + * make_heuristic_context must take an explicit TrackType rather than + * dereferencing Moves::trackp (which starts as nullptr). Callers pass the + * bound track; snapshots must come from that argument, not a nullable member. + */ +TEST_F(MovesTest, MakeHeuristicContextSnapshotsExplicitTrack) +{ + const TrackType tr = poisoned_track(); + ASSERT_EQ(moves->trackp, nullptr); + + const HeuristicContext ctx = context_for_hand_rel(*moves, tr, /*hand_rel=*/3); + + for (int s = 0; s < DDS_SUITS; ++s) + EXPECT_EQ(ctx.removed_ranks[s], 0x10 + s) << "suit=" << s; + EXPECT_EQ(ctx.lead0_rank, 14); + EXPECT_EQ(ctx.move1_rank, 12); + EXPECT_EQ(ctx.move1_suit, 2); + EXPECT_EQ(ctx.high1, 1); + EXPECT_EQ(ctx.move2_rank, 9); + EXPECT_EQ(ctx.move2_suit, 3); + EXPECT_EQ(ctx.high2, 2); + EXPECT_EQ(ctx.trackp, &tr); +} + +// HeuristicContext holds references to tpos / best_move / best_move_tt; the +// helper must bind them to storage that outlives the returned context. +TEST_F(MovesTest, MakeHeuristicContextBindsStableRefs) +{ + const TrackType tr = poisoned_track(); + const HeuristicContext ctx = context_for_hand_rel(*moves, tr, /*hand_rel=*/0); + + EXPECT_EQ(&ctx.tpos, &kTpos); + EXPECT_EQ(&ctx.best_move, &kBest); + EXPECT_EQ(&ctx.best_move_tt, &kBestTt); +} + +// Leading hand: move[0..2] are not played yet — leave trick snapshots at 0 +// even if the TrackType buffer holds stale/poisoned values. +TEST_F(MovesTest, MakeHeuristicContextLeadHandOmitsUnsetTrickCards) +{ + const TrackType tr = poisoned_track(); + const HeuristicContext ctx = context_for_hand_rel(*moves, tr, /*hand_rel=*/0); + + EXPECT_EQ(ctx.lead0_rank, 0); + EXPECT_EQ(ctx.move1_rank, 0); + EXPECT_EQ(ctx.move1_suit, 0); + EXPECT_EQ(ctx.high1, 0); + EXPECT_EQ(ctx.move2_rank, 0); + EXPECT_EQ(ctx.move2_suit, 0); + EXPECT_EQ(ctx.high2, 0); +} + +// Second hand: only the lead card is defined. +TEST_F(MovesTest, MakeHeuristicContextSecondHandSnapshotsOnlyLead) +{ + const TrackType tr = poisoned_track(); + const HeuristicContext ctx = context_for_hand_rel(*moves, tr, /*hand_rel=*/1); + + EXPECT_EQ(ctx.lead0_rank, 14); + EXPECT_EQ(ctx.move1_rank, 0); + EXPECT_EQ(ctx.move1_suit, 0); + EXPECT_EQ(ctx.high1, 0); + EXPECT_EQ(ctx.move2_rank, 0); + EXPECT_EQ(ctx.move2_suit, 0); + EXPECT_EQ(ctx.high2, 0); +} + +// Third hand: lead + second-hand card/high are defined; move[2] is not. +TEST_F(MovesTest, MakeHeuristicContextThirdHandSnapshotsThroughMove1) +{ + const TrackType tr = poisoned_track(); + const HeuristicContext ctx = context_for_hand_rel(*moves, tr, /*hand_rel=*/2); + + EXPECT_EQ(ctx.lead0_rank, 14); + EXPECT_EQ(ctx.move1_rank, 12); + EXPECT_EQ(ctx.move1_suit, 2); + EXPECT_EQ(ctx.high1, 1); + EXPECT_EQ(ctx.move2_rank, 0); + EXPECT_EQ(ctx.move2_suit, 0); + EXPECT_EQ(ctx.high2, 0); +} + +namespace +{ + +// Compact deal for MoveGen*: lead hand has only a few cards so the move list +// stays within MovePlyType::move[14]. The Init-only sample (0x3fff in every +// suit) overflows that buffer and trips ASan. +struct CompactDeal +{ + unsigned short rank_in_suit[DDS_HANDS][DDS_SUITS]{}; + Pos tpos{}; +}; + +auto popcount16(unsigned short bits) -> int +{ + int n = 0; + for (; bits != 0; bits = static_cast(bits & (bits - 1))) + ++n; + return n; +} + +auto make_compact_deal() -> CompactDeal +{ + CompactDeal d{}; + // Hand 0 (lead): AK of suit 0, Q of suit 1. + d.rank_in_suit[0][0] = 0x1800; + d.rank_in_suit[0][1] = 0x0400; + // Other hands: one card each for length/winner lookups. + d.rank_in_suit[1][0] = 0x0200; + d.rank_in_suit[1][2] = 0x0100; + d.rank_in_suit[2][0] = 0x0080; + d.rank_in_suit[3][1] = 0x0040; + + for (int h = 0; h < DDS_HANDS; ++h) + { + for (int s = 0; s < DDS_SUITS; ++s) + { + d.tpos.rank_in_suit[h][s] = d.rank_in_suit[h][s]; + d.tpos.length[h][s] = + static_cast(popcount16(d.rank_in_suit[h][s])); + d.tpos.aggr[s] = + static_cast(d.tpos.aggr[s] | d.rank_in_suit[h][s]); + } + } + return d; +} + +auto move_list_weights(const Moves& m, const int tricks, const int hand_rel, + const int n) -> std::vector +{ + EXPECT_GT(n, 0); + EXPECT_LE(n, 14); + std::vector weights; + weights.reserve(static_cast(n)); + for (int i = 0; i < n; ++i) + weights.push_back(m.moveList[tricks][hand_rel].move[i].weight); + return weights; +} + +} // namespace + +// MoveGen0 must range-check trump before reading winner[trump], matching the +// legacy heuristic dispatcher. Out-of-range values behave like no-trump. +TEST_F(MovesTest, MoveGen0OutOfRangeTrumpMatchesNoTrump) +{ + static RelRanksType rel[8192] = {}; + constexpr int tricks = 5; + const CompactDeal deal = make_compact_deal(); + const MoveType best{}; + + auto run = [&](const int trump) { + auto local = std::make_unique(); + local->Init(tricks, 0, nullptr, nullptr, deal.rank_in_suit, trump, 0); + const int n = local->MoveGen0(tricks, deal.tpos, best, best, rel); + return move_list_weights(*local, tricks, 0, n); + }; + + const auto nt = run(DDS_NOTRUMP); + // DDS_NOTRUMP == DDS_SUITS, so use values strictly outside [0, DDS_SUITS). + for (const int bad_trump : {-1, DDS_SUITS + 1, 99}) + { + const auto bad = run(bad_trump); + EXPECT_EQ(bad, nt) << "trump=" << bad_trump; + } +} + +// MoveGen123 likewise must not index winner[trump] when trump is out of range. +TEST_F(MovesTest, MoveGen123OutOfRangeTrumpMatchesNoTrump) +{ + constexpr int tricks = 5; + constexpr int hand_rel = 1; + constexpr int lead_suit = 0; + const CompactDeal deal = make_compact_deal(); + + auto run = [&](const int trump) { + auto local = std::make_unique(); + local->Init(tricks, 0, nullptr, nullptr, deal.rank_in_suit, trump, 0); + local->track[tricks].lead_suit = lead_suit; + local->track[tricks].move[0] = ExtCard{lead_suit, 8, 0}; + const int n = local->MoveGen123(tricks, hand_rel, deal.tpos); + return move_list_weights(*local, tricks, hand_rel, n); + }; + + const auto nt = run(DDS_NOTRUMP); + for (const int bad_trump : {-1, DDS_SUITS + 1, 99}) + { + const auto bad = run(bad_trump); + EXPECT_EQ(bad, nt) << "trump=" << bad_trump; + } +} + +// Hoisted make_heuristic_context must not observe stale suit/lastNumMoves from +// a prior call. Poison those members; MoveGen should reset them before +// snapshotting so weights match a clean instance. +TEST_F(MovesTest, MoveGenResetsSnapshottedFieldsBeforeHeuristicContext) +{ + static RelRanksType rel[8192] = {}; + constexpr int tricks = 5; + constexpr int lead_suit = 0; + CompactDeal deal = make_compact_deal(); + // Force a void-in-lead path for hand_rel=1 so suit/lastNumMoves are used + // by the void weight functions after the hoisted context is built. + deal.rank_in_suit[1][lead_suit] = 0; + deal.tpos.rank_in_suit[1][lead_suit] = 0; + deal.tpos.length[1][lead_suit] = 0; + const MoveType best{}; + + auto run_gen0 = [&](Moves& m) { + m.Init(tricks, 0, nullptr, nullptr, deal.rank_in_suit, DDS_NOTRUMP, 0); + return move_list_weights( + m, tricks, 0, m.MoveGen0(tricks, deal.tpos, best, best, rel)); + }; + auto run_gen123 = [&](Moves& m) { + m.Init(tricks, 0, nullptr, nullptr, deal.rank_in_suit, DDS_NOTRUMP, 0); + m.track[tricks].lead_suit = lead_suit; + m.track[tricks].move[0] = ExtCard{lead_suit, 8, 0}; + return move_list_weights( + m, tricks, 1, m.MoveGen123(tricks, /*handRel=*/1, deal.tpos)); + }; + + Moves clean; + const auto gen0_clean = run_gen0(clean); + const auto gen123_clean = run_gen123(clean); + + Moves poisoned; + poisoned.suit = 3; + poisoned.lastNumMoves = 99; + poisoned.leadSuit = 2; + EXPECT_EQ(run_gen0(poisoned), gen0_clean); + poisoned.suit = 3; + poisoned.lastNumMoves = 99; + poisoned.leadSuit = 2; + EXPECT_EQ(run_gen123(poisoned), gen123_clean); +} + /** * @section Performance Tests */ diff --git a/specs/heuristic-sorting.md b/specs/heuristic-sorting.md index 3b268593..d7af3957 100644 --- a/specs/heuristic-sorting.md +++ b/specs/heuristic-sorting.md @@ -1,7 +1,7 @@ --- capability: heuristic-sorting owners: [heuristic_sorting] -last-updated: 2026-07-18 +last-updated: 2026-07-24 --- # Heuristic Sorting @@ -37,12 +37,12 @@ solve. `doc/heuristic-sorting.md` is narrative intent (including lead/void per-suit top-card bonuses) and may lag the code; when they disagree, trust the implementation. -- **`call_heuristic` takes a pre-built `HeuristicContext`.** The caller constructs - one context (position, move array, best moves, relative ranks, and cached - per-trick snapshots such as `removed_ranks`, `move1_rank`, `high1`) and passes - it by const reference. Implementations `const_cast` and mutate move weights - in place. There is a single free overload — the older parameterized form is - gone. +- **`call_heuristic` takes a pre-built `HeuristicContext&` and a `WeightCase`.** + The caller constructs one context (position, move array, best moves, relative + ranks, and cached per-trick snapshots such as `removed_ranks`, `move1_rank`, + `high1`) and passes it by mutable reference; weighting writes through + `context.mply`. Move generation (`MoveGen0` / `MoveGen123`) always passes the + precomputed `WeightCase` so the case is not re-derived per suit. - **`TrackType` is shared per-trick position state** defined here and consumed by both [move-generation](move-generation.md) and the heuristics. `Moves` owns the `track[]` array and mutates `trackp`; heuristics read the snapshots copied into @@ -53,8 +53,8 @@ solve. ## Key entry points - `library/src/heuristic_sorting/heuristic_sorting.hpp` — `HeuristicContext`, - `TrackType`, and `call_heuristic(const HeuristicContext&)`. Doxygen documents the - fields. + `TrackType`, `WeightCase`, and `call_heuristic(context, weight_case)`. + Doxygen documents the fields. - `library/src/heuristic_sorting/heuristic_sorting.cpp` + `internal.hpp` — the per-situation weighting helpers. - Narrative algorithm: `doc/heuristic-sorting.md`.