You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Add analyse_play to the modern C++ API, C-ABI shim, and Python bindings - #441
Adds a context-aware analyse_play(SolverContext&, ...) to the modern C++ API, extracted from legacy AnalysePlayBin's body (which now delegates to it, preserving external behavior).
Mirrors it through the pure-C ABI shim as dds_c_analyse_play, plus a new dds_c_convert_play_from_pbn converter (PBN play trace → binary), following the shim's existing no-PBN-twin pattern; regenerates the JNI export lists accordingly.
Adds a Python analyse_play (binary) binding and refactors analyse_play_pbn to drop its call into legacy AnalysePlayPBN, converting PBN→binary in C++ and calling the modern analyse_play directly instead.
Updates specs/dds-public-api.md to reflect the shim's expanded surface, and fixes two docstrings that overpromised ValueError for illegal plays (they actually raise RuntimeError).
Parallelizing analyse_all_plays_pbn was explicitly scoped out as a follow-up (deferred, not implemented here).
Test plan
bazel test //library/tests/system:context_equivalence_test — context vs. legacy equivalence
bazel test //library/tests/solve_board:analyse_play_consistency_test — regression gate for the AnalysePlayBin extraction
bazel test //library/tests:dds_c_api_test — shim null-guards, functional, and converter tests
bazel test //jni/tests:export_set_test — export-list regeneration verified
bazel test //python:analyse_test — new and refactored Python bindings
AnalysePlayBin/AnalysePlayPBN existed only in the legacy C API, with no
SolverContext-taking counterpart and no entry in the pure-C ABI shim
(dds_c_api.h), unlike SolveBoard/CalcDDtable. Add analyse_play(ctx, ...) as
the modern C++ entry point (extracted from AnalysePlayBin's body, which now
delegates to it), mirror it through the shim as dds_c_analyse_play plus a
dds_c_convert_play_from_pbn converter, and give Python a binary analyse_play
binding while refactoring analyse_play_pbn to use the modern API exclusively
instead of calling legacy AnalysePlayPBN.
Parallelizing analyse_all_plays_pbn is intentionally deferred (see the
follow-up note left in the implementation plan) -- out of scope here.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Documents the dds_c_analyse_play and dds_c_convert_play_from_pbn
additions in the shim's covered-surface list and explains that the
new converter follows the existing no-PBN-twin rule.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rors
Both docstrings claimed ValueError for an invalid play, but an illegal
play (card not in hand, etc.) returns RETURN_PLAY_FAULT, which
throw_on_dds_error does not map to ValueError -- it falls through to
RuntimeError. Correct both to document RuntimeError for play faults,
keeping ValueError scoped to the Python-level format/validation errors
that actually raise it.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Aligning continuation params to the opening paren landed at a 34-space
indent (34 % 4 == 2), tripping the repo's indentation check
(cpp_indentation_test.py), which only runs its full-tree scan outside
the Bazel sandbox -- that's why CI caught it but the local bazel test
run didn't. Reformat both new declarations/definitions to the flat
4-space continuation style dll.h already uses, which sidesteps the
alignment arithmetic entirely.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Drop the "modern"/"legacy"/"no longer" framing added for analyse_play
and analyse_play_pbn -- this doc should describe what the functions do
today, not how that differs from history. That contrast belongs in
release notes, not the API reference.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
The new source is omitted from both Visual Studio projects, causing link failures, and missing play-trace validation in the binary C entry point permits out-of-bounds memory access.
…/remainCards
dds_c_analyse_play (and every other caller: legacy AnalysePlayBin, the
dds_api.hpp forwarder, the Python binding) forwarded play.suit/play.rank
straight into bit_map_rank[play.rank] and dl.remainCards[player][play.suit]
with no range check. A garbage or malicious suit/rank is an out-of-bounds
array access, not a C++ exception, so dds_c_api.cpp's catch-all could not
have contained it.
Push the fix into the one shared analyse_play() implementation so every
caller gets it: reject play.number outside [0, 52] and any play.suit/
play.rank entry outside [0, DDS_SUITS) / [2, 14] up front, returning
RETURN_PLAY_FAULT instead of touching the arrays. Add coverage both at
the shared-implementation level (context_equivalence_test.cpp) and at
the dds_c_api.cpp shim boundary where this was flagged
(dds_c_api_test.cpp), since that's the entry point an external FFI
caller would actually hit.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Bazel's source glob picked it up automatically, but DDS.vcxproj and
dds_native.vcxproj both compile play_analyser.cpp (now a two-line
delegate) and never gained the extracted analyse_play_adapter.cpp,
leaving analyse_play with no linked definition on the Windows VS build.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ership
tricks[i] was documented as "the trick count for the side to play",
which reads as the side on lead -- but it's actually a fixed
partnership (the one opposite the hand that led to the first trick)
tracked throughout the whole trace, not the alternating side to move.
Clarify this in both Python docstrings, the dds_c_analyse_play header
comment, and docs/python_interface.md.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The C API header does not expose the new play input and result layouts, preventing C consumers from allocating these types and header-based binding generators from reading their definitions.
No open issues 🎉
1 previously missed issue
In code that hasn't changed since last review
Expose play structs in the shared C data-types header
library/src/api/dds_c_api.h:124
A consumer including only dds_c_api.h cannot allocate the new play inputs or results: PlayTraceBin, PlayTracePBN, and SolvedPlay are defined only in dds_data_types.hpp, leaving header-based binding generators without their layouts. Move these three POD definitions into dds_c_data_types.h and remove the original definitions; the C++ header already includes that shared header. This also fixes dds_c_convert_play_from_pbn below.
…pes header
dds_c_api.h now takes these three types by pointer (dds_c_analyse_play,
dds_c_convert_play_from_pbn), but they were defined only in
dds_data_types.hpp, unreachable from dds_c_api.h alone -- leaving C
consumers and header-based binding generators (jextract) unable to see
their layout.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@tameware, here is another one exposing more functions through the C API. Note that I deferred the decision about synchronising documentation and actual error types as I think this might be a wider problem.
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I think I've mentioned before that in the long term, I'm not sure we should support the binary interface. There can't be any measurable efficiency gain, and sticking to text-oriented standards such as PBN for externals will make our clients' implementation and debugging easier.
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
Clean Copilot reviewCopilot reviewed and had neither new comments nor new suppressed comments.
3 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.
Summary
analyse_play(SolverContext&, ...)to the modern C++ API, extracted from legacyAnalysePlayBin's body (which now delegates to it, preserving external behavior).dds_c_analyse_play, plus a newdds_c_convert_play_from_pbnconverter (PBN play trace → binary), following the shim's existing no-PBN-twin pattern; regenerates the JNI export lists accordingly.analyse_play(binary) binding and refactorsanalyse_play_pbnto drop its call into legacyAnalysePlayPBN, converting PBN→binary in C++ and calling the modernanalyse_playdirectly instead.specs/dds-public-api.mdto reflect the shim's expanded surface, and fixes two docstrings that overpromisedValueErrorfor illegal plays (they actually raiseRuntimeError).analyse_all_plays_pbnwas explicitly scoped out as a follow-up (deferred, not implemented here).Test plan
bazel test //library/tests/system:context_equivalence_test— context vs. legacy equivalencebazel test //library/tests/solve_board:analyse_play_consistency_test— regression gate for theAnalysePlayBinextractionbazel test //library/tests:dds_c_api_test— shim null-guards, functional, and converter testsbazel test //jni/tests:export_set_test— export-list regeneration verifiedbazel test //python:analyse_test— new and refactored Python bindings🤖 Generated with Claude Code