Repository navigation
bridge-cpp: static and instance methods - #4088
Merged
Merged
Conversation
Bridge-week step 11, re-landed from -full under the current machinery. Classes with BAML methods emit real member functions: statics as static members, instance methods as const members encoding *this as the self kwarg (deep-copy semantics, python parity), each with its _async sibling returning baml::future - typed throws, opts structs (nested in the class, owner-qualified in the out-of-line bindings.cc definitions), and Raises: doc blocks all ride the existing callable machinery. Dispatch goes to the class wire symbol plus the method's source member token. The naming pool grows CppNameKind::Method (tag appended; existing typed-hash tags never move), and class emission ordering waits on method optional-param types during the first fixed-point pass so nested opts structs never reference an undeclared type. Ports: test_methods_on_classes (6 engine round-trip cases), opt_box_method_matrix, and the method Raises: blocks python pins in its .pyi stubs.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe C++ SDK generator now emits static and instance class methods with synchronous and asynchronous variants, method-specific naming and dispatch, qualified bindings, receiver encoding, option structs, and tests covering calls, optional arguments, symbol reachability, and Raises documentation. ChangesC++ class method generation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CppCaller
participant GeneratedGreeterBinding
participant BAMLDispatch
CppCaller->>GeneratedGreeterBinding: invoke Greeter method
GeneratedGreeterBinding->>BAMLDispatch: send method key, self, and arguments
BAMLDispatch-->>GeneratedGreeterBinding: return sync result or async future
GeneratedGreeterBinding-->>CppCaller: return value
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Binary size checks failed❌ 4 violations · ✅ 3 passed
Details & how to fixViolations:
Add/update baselines:
[artifacts.baml-cli]
file_bytes = 19545088
stripped_bytes = 19545136
gzip_bytes = 9331880
[artifacts.bridge_wasm]
file_bytes = 16160508
gzip_bytes = 4402886
[artifacts.baml-cli]
file_bytes = 21075968
stripped_bytes = 21075968
gzip_bytes = 9542344
[artifacts.baml-cli]
file_bytes = 25272400
stripped_bytes = 25272392
gzip_bytes = 10732613Generated by |
Parity-workflow finding: python's test_imports pins all five raises_test symbols; InferredThrow and PureLen were never compiled against in the C++ suite (header scrapes are namespace-blind), so a wrong-namespace codegen regression would pass. The established (void)sizeof / (void)& reachability pattern closes it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
baml_language/sdks/cpp/sdkgen_cpp/src/lib.rs (1)
174-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit tests for the new method-dispatch path
baml_language/sdks/cpp/sdkgen_cpp/src/naming.rs: coverCppNameKind::Methodsotag()stays10andrenders_bare()stays true.baml_language/sdks/cpp/sdkgen_cpp/src/lib.rs: coveremit_callable/pass 3 so methodcall_fqnusesclass.memberand static vs. instance methods land in the right vectors.🤖 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 `@baml_language/sdks/cpp/sdkgen_cpp/src/lib.rs` around lines 174 - 221, Add unit coverage in baml_language/sdks/cpp/sdkgen_cpp/src/naming.rs:128-177 for CppNameKind::Method, asserting tag() remains 10 and renders_bare() remains true. Add pass-3 tests in baml_language/sdks/cpp/sdkgen_cpp/src/lib.rs:174-221 covering emit_callable and method dispatch, verifying call_fqn uses class.member and static_methods versus instance_methods receive the correct emitted methods.Source: Coding guidelines
🤖 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 `@baml_language/sdks/cpp/sdkgen_cpp/src/lib.rs`:
- Around line 174-221: Add unit coverage in
baml_language/sdks/cpp/sdkgen_cpp/src/naming.rs:128-177 for CppNameKind::Method,
asserting tag() remains 10 and renders_bare() remains true. Add pass-3 tests in
baml_language/sdks/cpp/sdkgen_cpp/src/lib.rs:174-221 covering emit_callable and
method dispatch, verifying call_fqn uses class.member and static_methods versus
instance_methods receive the correct emitted methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2278b60d-8085-4dae-baa4-da18b4bd62d6
📒 Files selected for processing (5)
baml_language/sdk_tests/crates/cpp/function_calls/customizable/tests/test_methods_on_classes.ccbaml_language/sdk_tests/crates/cpp/function_calls/customizable/tests/test_optional_args.ccbaml_language/sdk_tests/crates/cpp/function_calls/customizable/tests/test_raises.ccbaml_language/sdks/cpp/sdkgen_cpp/src/lib.rsbaml_language/sdks/cpp/sdkgen_cpp/src/naming.rs
This branch was previously deployed
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
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.
Bridge-week step 11, re-landed from
-fullunder all the current machinery.What
*thisas theselfkwarg (deep-copy semantics, python parity); dispatch = class wire symbol +.member.OptBox::make_opts, owner-qualified in the out-of-line bindings.cc definitions),/// Raises:doc blocks, and_asyncsiblings returningbaml::future<T, variant<...>>all ride the existing callable machinery — zero runtime changes.CppNameKind::Method(tag appended; existing typed-hash tags never move).Tests
test_methods_on_classes.cc: python port, 6 engine round-trip cases (static create, instance who/greet, all async siblings).opt_box_method_matrixintest_optional_args.cc: static + instance methods with opts structs.method_raises_blocksintest_raises.cc: theRaises:blocks python pins in its .pyi stubs.All 12
sdk_test_cppfixture jobs pass locally.Summary by CodeRabbit
New Features
Raises) details.Tests
Raisesdocumentation formatting/reachability.