Skip to content

Add PBN bindings to the Java FFM shim - #401

Open
zzcgumn wants to merge 1 commit into
developfrom
feature/pbn_capable_c_exports
Open

zzcgumn wants to merge 1 commit into
developfrom
feature/pbn_capable_c_exports

Conversation

@zzcgumn

@zzcgumn zzcgumn commented Sep 22, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • The core ask in Expose PBN-capable C exports #284 (PBN-capable C exports: dds_solve_board_pbn, dds_calc_par_pbn, dds_calc_dd_table_pbn on the modern C++ API and their dds_c_* shim twins) was already implemented and tested.
  • The Java FFM binding (Dds.java) had no PBN support at all, not even for the pre-existing dds_c_calc_dd_table_pbn. Add DealPBN/DdTableDealPBN memory layouts and downcall handles for all three PBN twins, with TDD smoke-test coverage cross-checked against the same reference fixture used by dds_c_api_test.
  • Fix specs/dds-public-api.md, which still claimed the shim had only one PBN twin.
  • Addresses Expose PBN-capable C exports #284.

Test plan

  • bazel test //jni:dds_ffm_smoke_test — passes, including new PBN checks.
  • bazel test //jni:dds_ffm_embedded_smoke_test //jni/tests:export_set_test //jni/tests:jar_self_contained_test //library/tests:dds_c_api_test — all pass.

🤖 Generated with Claude Code

#Closes #284

dds_c_solve_board_pbn and dds_c_calc_par_pbn (and dds_c_calc_dd_table_pbn)
were already implemented and tested in the C shim per #284, but the Java
FFM binding (Dds.java) had no PBN support at all. Add the DealPBN /
DdTableDealPBN layouts and downcall handles, with smoke-test coverage
cross-checked against the same reference fixture used by dds_c_api_test.

Also fix specs/dds-public-api.md, which still claimed the shim had only
one PBN twin.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

A documentation coverage claim remains unresolved, so final human review is recommended.

Review effort: Lite
Findings: None

What changed in this PR

Adds Java FFM bindings and smoke tests for all three PBN APIs, and updates the public API documentation.

Changes:

  • Added PBN layouts, handles, and methods.
  • Added PBN solve, table, and par smoke tests.
  • Updated shim capability documentation.
File Description
specs/​dds-public-api.md Documents the PBN shim surface.
jni/​java/​org/​dds/​ffm/​DdsSmokeTest.java Tests PBN operations.
jni/​java/​org/​dds/​ffm/​Dds.java Adds PBN FFM bindings.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@zzcgumn

zzcgumn commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator Author

A documentation coverage claim remains unresolved, so final human review is recommended.

I am not sure what Copilot means by this.

@tameware

Copy link
Copy Markdown
Collaborator

Nor am I! My guess is that it just wants me to look.

@tameware

Copy link
Copy Markdown
Collaborator

I asked GPT 5.4 (via a Copilot Agent) for clarification:

It means Copilot did not find a concrete bug, but it also could not verify the PR’s documentation claim with confidence.

In this PR, that likely refers to the statement that the docs/spec now correctly describe the Java shim’s PBN support. Copilot is basically saying:

  • “I see documentation changed.”
  • “I can’t prove the docs are fully complete/correct.”
  • “A human should make the final call.”

So this is a soft warning, not a real review finding. The key clue is:

  • Findings: None
  • Needs a closer look
  • final human review is recommended

In practice, it usually means: please manually confirm the updated doc matches the actual exported API surface.

@tameware tameware left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

This is fine if we want to make the existing PBN twins available in every API. Have we considered deprecating the twins and providing a PBN to Deal convertor instead?

@zzcgumn

zzcgumn commented Sep 23, 2026

Copy link
Copy Markdown
Collaborator Author

Have we considered deprecating the twins and providing a PBN to Deal convertor instead?

I will look into this suggestion. A converter might be a cleaner solution, and it shrinks the number of functions we support as part of the API.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Expose PBN-capable C exports

3 participants