Skip to content

[ffigen] add Support passing C++ classes by value - #3677

Merged
liamappelbe merged 5 commits into
dart-lang:mainfrom
Manar-Elhabbal7:cpp-pass-by-value
Oct 6, 2026
Merged

[ffigen] add Support passing C++ classes by value #3677
liamappelbe merged 5 commits into
dart-lang:mainfrom
Manar-Elhabbal7:cpp-pass-by-value

Conversation

@Manar-Elhabbal7

@Manar-Elhabbal7 Manar-Elhabbal7 commented Sep 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What does this PR introduce?

Adds support for passing and returning C++ classes by value:

  • By-value arguments are passed as pointers in C++ glue code and dereferenced with *.
  • By-value returns are allocated with new in C++ glue code and wrapped in Dart with takeOwnership: true.
  • Removed parser skip checks in classdecl_parser.dart.

I followed a TDD approach: added by-value test methods in cpp_class_test and verified they failed before implementing the fix. And it passed successfully : )

Fixes #3603

@Manar-Elhabbal7

Manar-Elhabbal7 commented Sep 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Hello @liamappelbe PTAL
this is my first PR in the ffigen . so if there are any feedbacks this will help me alot thanks : )

@liamappelbe
liamappelbe self-requested a review September 21, 2026 01:10
@codecov

codecov Bot commented Sep 21, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 69.81132% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.89%. Comparing base (f7ffe96) to head (137d5e5).

Files with missing lines Patch % Lines
pkgs/ffigen/lib/src/code_generator/typealias.dart 0.00% 8 Missing ⚠️
pkgs/ffigen/lib/src/code_generator/type.dart 40.00% 6 Missing ⚠️
pkgs/ffigen/lib/src/code_generator/cpp_class.dart 96.66% 1 Missing ⚠️
pkgs/ffigen/lib/src/code_generator/pointer.dart 80.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3677      +/-   ##
==========================================
- Coverage   87.93%   87.89%   -0.05%     
==========================================
  Files         310      310              
  Lines       19799    19831      +32     
==========================================
+ Hits        17411    17431      +20     
- Misses       2388     2400      +12
Flag Coverage Δ
ffigen 91.36% <69.81%> (-0.13%) ⬇️
native_pkgs_macos 86.34% <ø> (ø)
native_pkgs_ubuntu 71.97% <ø> (ø)
native_pkgs_windows 75.32% <ø> (ø)
objective_c 62.86% <0.00%> (-0.26%) ⬇️
swiftgen 80.41% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
code_assets 94.96% <ø> (ø)
data_assets 91.56% <ø> (ø)
ffi 100.00% <ø> (ø)
ffigen 91.45% <69.81%> (-0.13%) ⬇️
hooks 86.40% <ø> (ø)
hooks_runner 89.72% <ø> (ø)
jni 66.34% <ø> (ø)
jni_flutter ∅ <ø> (∅)
jni_util 25.00% <ø> (ø)
jnigen 82.85% <ø> (ø)
json_syntax_generator ∅ <ø> (∅)
native_test_helpers 85.71% <ø> (ø)
native_toolchain_c 95.94% <ø> (ø)
objective_c 79.53% <ø> (ø)
pub_formats ∅ <ø> (∅)
record_use 81.48% <ø> (ø)
snippet_tool 66.23% <ø> (ø)
swift2objc 92.95% <ø> (ø)
swiftgen 80.41% <ø> (ø)
test_case_selector 97.28% <ø> (ø)
Files with missing lines Coverage Δ
...rc/header_parser/sub_parsers/classdecl_parser.dart 93.33% <ø> (-0.34%) ⬇️
pkgs/ffigen/lib/src/code_generator/cpp_class.dart 96.33% <96.66%> (+1.95%) ⬆️
pkgs/ffigen/lib/src/code_generator/pointer.dart 91.09% <80.00%> (-0.40%) ⬇️
pkgs/ffigen/lib/src/code_generator/type.dart 81.57% <40.00%> (-6.30%) ⬇️
pkgs/ffigen/lib/src/code_generator/typealias.dart 92.30% <0.00%> (-7.70%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

liamappelbe
liamappelbe reviewed Sep 23, 2026
View reviewed changes

@liamappelbe liamappelbe 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.

You've chosen a challenging feature as your first PR in ffigen 😉

High level comments, before I do a full review:

  • It's not clear to me how your changes can tell the difference between a class by pointer and a class by value. My understanding is that CppClass inherently represents a pointer to a class (though I didn't write this code, so I could be wrong; maybe class pointers are represented by Pointer<CppClass>. How exactly is a Foo* represented in the AST differently to a Foo?
  • I was expecting there to be a flag on CppClass that says it's by-value, rather than these ad-hoc by value checks.
  • That would allow you to fold the by-value/by-pointer special casing into getNativeType. Right now you have it re-implemented in several locations.
  • Consequently, your current approach will not work for other usages of CppClass, such as global variables or top-level-functions.

@Manar-Elhabbal7

Manar-Elhabbal7 commented Sep 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

You've chosen a challenging feature as your first PR in ffigen 😉

High level comments, before I do a full review:

  • It's not clear to me how your changes can tell the difference between a class by pointer and a class by value. My understanding is that CppClass inherently represents a pointer to a class (though I didn't write this code, so I could be wrong; maybe class pointers are represented by Pointer<CppClass>. How exactly is a Foo* represented in the AST differently to a Foo?
  • I was expecting there to be a flag on CppClass that says it's by-value, rather than these ad-hoc by value checks.
  • That would allow you to fold the by-value/by-pointer special casing into getNativeType. Right now you have it re-implemented in several locations.
  • Consequently, your current approach will not work for other usages of CppClass, such as global variables or top-level-functions.

Thanks, @liamappelbe, for your initial feedback. Yeah, I think it's tricky :)

First

To clarify how Foo* vs Foo is structured in the AST:

  • Foo* (by pointer) is represented as CppClassPointerType (a subclass of PointerType).
  • Foo (by value) is represented directly as CppClass.
  • std::unique_ptr<Foo> is represented as CppUniquePtrType.

Because they are distinct AST types, CppClassPointerType.getNativeType() returns
Foo*, while CppClass.getNativeType() returns Foo.

BTW I took this approach as u mentioned in the issue description

We could support passing C++ classes by value by converting it to a pointer in out C++ glue code. By-value > returns would need to be caught and stored in a new allocated pointer. By value args can just be defererenced with >*. I don't think we'd need any new infra on the Dart side.

Second

Why not use a flag in CppClass ?

As I mentioned before, CppClass represents the by-value class, while CppClassPointerType represents the pointer, so an extra boolean flag isn't needed.

Third

Completely agree with you at this point. I suggest adding glue-generation methods to Type (getNativeGlueType, generateGlueCallArg, and generateGlueReturn) and overriding them in CppClass and CppUniquePtrType. This encapsulates all the by-value/by-pointer conversions cleanly inside the type classes using polymorphism.

Fourth

I think now, as I mentioned in point three Because the glue logic is now part of the shared Type class hierarchy, any future generator will simply call p.type.generateGlueCallArg(p.name) and p.type.generateGlueReturn(...) and it will automatically work!

What are your thoughts now? I will test my approach also and tell u what the results I have got

@liamappelbe

liamappelbe commented Sep 25, 2026

Copy link
Copy Markdown
Contributor

What are your thoughts now? I will test my approach also and tell u what the results I have got

Sounds good. But call the functions getExternCType (should default to calling getNativeType), convertExternCTypeToNativeType, and convertNativeTypeToExternCType. If you take a look at type.dart, you can see we already have a bunch of variants of getFooType/convertFooTypeToBarType methods. If you need it, you might also want to add a sameNativeAndExternCType getter.

@Manar-Elhabbal7
Manar-Elhabbal7 force-pushed the cpp-pass-by-value branch 2 times, most recently from 66744b2 to bb71cf7 Compare September 25, 2026 20:26
@Manar-Elhabbal7

Manar-Elhabbal7 commented Sep 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

What are your thoughts now? I will test my approach also and tell u what the results I have got

Sounds good. But call the functions getExternCType (should default to calling getNativeType), convertExternCTypeToNativeType, and convertNativeTypeToExternCType. If you take a look at type.dart, you can see we already have a bunch of variants of getFooType/convertFooTypeToBarType methods. If you need it, you might also want to add a sameNativeAndExternCType getter.

Very good. I have implemented my approach and take your reviews in my mind. PTAL, and if there are any feedbacks, you can give them. thanks :)

@liamappelbe

liamappelbe commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

@Manar-Elhabbal7 Please avoid using git push --force. It makes PRs much harder to review, because it breaks the "Changes since your last review" button.

@liamappelbe

liamappelbe commented Sep 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

You've got some analysis errors to fix, which prevent the tests from running. Make sure to run dart format and dart analyze before you push.

EDIT: The errors will go away if you sync to head.

@Manar-Elhabbal7

Manar-Elhabbal7 commented Sep 28, 2026

Copy link
Copy Markdown
Contributor Author

@Manar-Elhabbal7 Please avoid using git push --force. It makes PRs much harder to review, because it breaks the "Changes since your last review" button.

Ohh, sorry for that. It's my first time knowing this. noted :)

@Manar-Elhabbal7

Manar-Elhabbal7 commented Sep 28, 2026

Copy link
Copy Markdown
Contributor Author

You've got some analysis errors to fix, which prevent the tests from running. Make sure to run dart format and dart analyze before you push.

EDIT: The errors will go away if you sync to head.

Yeah, I have run both dart format and dart analyze, and there are no issues and no files edited. I think it's like you said to sync to head. I wish it now works well without CI failing

liamappelbe
liamappelbe reviewed Sep 30, 2026
View reviewed changes

@liamappelbe liamappelbe left a comment •
edited
Loading

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.

Looks pretty good, almost there. Can you add a test that checks memory management of passing args/returns by value? You can just add cases to pkgs/ffigen/test/native_cpp_test/memory_edge_cases_test.dart

Comment thread pkgs/ffigen/lib/src/code_generator/cpp_class.dart Resolved
Comment thread pkgs/ffigen/lib/src/code_generator/cpp_class.dart Resolved
@Manar-Elhabbal7

Manar-Elhabbal7 commented Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

Oh, @liamappelbe, I have clicked the update with the rebase button, and this made a force push. Sorry for that :(
Anyway, I have made your suggestions, PTAL

@Manar-Elhabbal7

Manar-Elhabbal7 commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

@liamappelbe PTAL

@liamappelbe
liamappelbe merged commit b66d081 into dart-lang:main Oct 6, 2026
61 of 62 checks passed
@Manar-Elhabbal7
Manar-Elhabbal7 deleted the cpp-pass-by-value branch October 6, 2026 06:54
copybara-service Bot pushed a commit to dart-lang/sdk that referenced this pull request Oct 7, 2026
@natebosch dart-scoped@luci-project-accounts.iam.gserviceaccount.com
[deps] rev core, dartdoc, http, native, shelf, test, tools, web
1d6b42b 
Revisions updated by `dart tools/rev_sdk_deps.dart`.

core (https://github.com/dart-lang/core/compare/08e4022..4d80610):
  4d80610a  Thu Oct 1 22:03:38 2026 +0000  dependabot[bot]  Bump the github-actions group with 5 updates (dart-lang/core#1005)

dartdoc (https://github.com/dart-lang/dartdoc/compare/2a040a8..5eb0f2c):
  5eb0f2c7  Thu Oct 1 19:03:48 2026 +0000  dependabot[bot]  Bump github/codeql-action/upload-sarif from 4.37.9 to 4.38.2 in the github-actions group (dart-lang/dartdoc#4291)

http (https://github.com/dart-lang/http/compare/d8e8e9c..585d433):
  585d433  Mon Oct 5 16:12:54 2026 -0700  Brian Quinlan  fix(cupertino_http): fix a websocket memory leak (dart-lang/http#1991)
  337adf3  Mon Oct 5 16:03:25 2026 -0700  Brian Quinlan  fix(cupertino_http): add `autoReleasePool` use to `CupertinoClient` (dart-lang/http#1990)
  47c57df  Tue Sep 29 14:07:01 2026 -0700  Brian Quinlan  fix(ok_http): fix `NetworkOnMainThreadException` (dart-lang/http#1995)
  d8cde5d  Tue Sep 29 14:06:44 2026 -0700  Brian Quinlan  fix test (dart-lang/http#1996)
  31056fe  Tue Sep 29 13:57:20 2026 -0700  Brian Quinlan  chore(cronet_http): prepare to release 1.10.0 (dart-lang/http#1993)
  9bc3a91  Tue Sep 29 13:53:25 2026 -0700  Brian Quinlan  fix(cronet_http): one global reference per `onReadCompleted` (dart-lang/http#1992)
  49ecd36  Mon Sep 28 14:20:26 2026 -0700  Brian Quinlan  fix(conformance): `supportsAbort` not considered in response (dart-lang/http#1994)

native (https://github.com/dart-lang/native/compare/27f7678..a2a6179):
  a2a617910  Tue Oct 6 13:53:17 2026 +0200  Daco Harkes  [jni_flutter] Bump version for publishing (dart-lang/native#3730)
  d3a99d66b  Tue Oct 6 19:46:34 2026 +1100  Liam Appelbe  [infra] Fix ffigen and jni_flutter (dart-lang/native#3724)
  b66d0815a  Tue Oct 6 04:28:35 2026 +0300  Manar Elhabbal  [ffigen] add Support passing C++ classes by value  (dart-lang/native#3677)
  f7ffe9667  Fri Oct 2 11:33:49 2026 +0200  Moritz  [native_toolchain_c] Don't pass /INCLUDE per symbol with a generated .def on Windows (dart-lang/native#3718)
  77a5e4169  Thu Oct 1 10:56:45 2026 +0300  Hassnaa Mohamed  [ffigen] Fix cpp method filtering (dart-lang/native#3696)
  cca065ca6  Thu Oct 1 10:17:12 2026 +1000  Liam Appelbe  [infra] Prepare to publish everything (dart-lang/native#3716)
  f13623ee3  Thu Oct 1 10:16:39 2026 +1000  Liam Appelbe  [jnigen] API docs (dart-lang/native#3715)
  be618790c  Thu Oct 1 10:05:26 2026 +1000  Liam Appelbe  [objective_c] API docs (dart-lang/native#3708)
  a18dd1148  Thu Oct 1 10:04:38 2026 +1000  Liam Appelbe  [ffigen] API docs (dart-lang/native#3707)
  6fd87c3b2  Tue Sep 29 19:01:11 2026 -0700  Brian Quinlan  [jni] Clarify `JBuffer.asUint8List` behavior (dart-lang/native#3704)
  6d5bbcfad  Wed Sep 30 09:35:47 2026 +1000  Liam Appelbe  [jni] API documentation (dart-lang/native#3706)
  fbcbc894b  Wed Sep 30 09:35:34 2026 +1000  Liam Appelbe  [ffigen] Document autorelease issues (dart-lang/native#3705)
  f99449ec9  Tue Sep 29 09:39:27 2026 +1000  Liam Appelbe  [infra] Add swift2objc to the workspace (dart-lang/native#3700)
  31494fa08  Tue Sep 29 09:38:59 2026 +1000  Liam Appelbe  [infra] Add jni_util to the workspace (dart-lang/native#3698)

shelf (https://github.com/dart-lang/shelf/compare/e5c8dc6..b2cb503):
  b2cb503  Wed Sep 30 22:53:07 2026 -0700  Kevin Moore  Resolve shelf.io.connection_info lazily (dart-lang/shelf#542)
  b70fafa  Tue Sep 29 13:37:08 2026 -0700  Kevin Moore  Match parameterless shelf_router routes without running a regex (dart-lang/shelf#544)

test (https://github.com/dart-lang/test/compare/4f92d53..d749b10):
  d749b104  Mon Oct 5 15:42:46 2026 -0700  Nate Bosch  Fail isolate suites when the test isolate exits (dart-lang/test#2775)
  da2157e9  Mon Oct 5 11:12:43 2026 -0700  Nate Bosch  Wait longer for the Chrome test tab to appear (dart-lang/test#2771)
  00bd0c99  Mon Oct 5 18:50:03 2026 +0300  Yusuf İhsan Görgel  Fix broken links in architecture.md (dart-lang/test#2774)
  3be2f547  Thu Oct 1 03:46:29 2026 +0000  dependabot[bot]  Bump the github-actions group with 4 updates (dart-lang/test#2773)
  40a5ad90  Tue Sep 29 12:07:31 2026 -0700  Nate Bosch  Delete compilation artifacts once they are no longer needed (dart-lang/test#2764)
  171ad0e5  Mon Sep 28 17:29:48 2026 -0700  Jacob MacDonald  export additional types from package:test_api/backend.dart (dart-lang/test#2745)
  52b45064  Mon Sep 28 15:04:08 2026 -0700  Nate Bosch  Tag browser and node tests, relax flaky timeouts (dart-lang/test#2770)

tools (https://github.com/dart-lang/tools/compare/d87eaf7..a66d517):
  a66d517d  Tue Oct 6 09:41:32 2026 -0700  Phil Quitslund  [analytics] `DASH__SUPPRESS_ANALYTICS` env handling (dart-lang/tools#2641)
  00c6e4ad  Fri Oct 2 17:17:49 2026 +0530  shivytyahoo  Fix VersionConstraint.intersection throwing on disjoint ranges (dart-lang/tools#2622)
  d523e1f8  Thu Oct 1 11:09:16 2026 +0200  Morgan :)  [unified_analytics] Append to the log file instead of rewriting it (dart-lang/tools#2613)
  6993cbad  Wed Sep 30 14:09:32 2026 +0300  Yusuf İhsan Görgel  [yaml] Improve error messages when mixing list and key-value syntax (dart-lang/tools#2610)
  068119b4  Tue Sep 29 23:21:03 2026 +0200  Lukas Klingsbo  [pubspec_parse] Support tag_pattern for git dependencies (dart-lang/tools#2565)
  7d859ed5  Mon Sep 28 20:28:23 2026 -0700  Kevin Moore  [api_summary] Add expectApiSummaryClean and --write/--check CLI flags (dart-lang/tools#2603)

web (https://github.com/dart-lang/web/compare/ec71a4c..63e6662):
  63e6662  Sat Oct 3 00:23:21 2026 +0000  Natalie Weizenbaum  Add a JSPair type (dart-lang/web#586)
  2d68d90  Fri Oct 2 20:26:48 2026 +0000  Natalie Weizenbaum  Use `js_interop` APIs in `js_interop_gen` (dart-lang/web#585)
  037823c  Thu Oct 1 21:59:21 2026 +0000  Natalie Weizenbaum  Add JSMap, JSSet, and JSSetLike types (dart-lang/web#571)
  b2d1d6b  Thu Oct 1 06:57:03 2026 +0000  dependabot[bot]  Bump the github-actions group with 3 updates (dart-lang/web#584)
  8dc8f75  Wed Sep 30 08:12:18 2026 -0700  Kevin Moore  [js_interop_gen] Split union/intersection AST types and extract TypeTransformer (dart-lang/web#580)
  751ccdf  Wed Sep 30 08:12:18 2026 -0700  Kevin Moore  [js_interop_gen] Decompose `Transformer._transformType` and deduplicate type caching (dart-lang/web#579)
  6f3dd4e  Wed Sep 30 08:12:17 2026 -0700  Kevin Moore  [js_interop_gen] Deduplicate `UnionType` and `IntersectionType` via `UnionOrIntersectionType` (dart-lang/web#578)
  d9f095e  Tue Sep 29 12:17:29 2026 -0700  Kevin Moore  [js_interop_gen] Remove unused JS interop extension types and bindings (dart-lang/web#577)
  07111ee  Tue Sep 29 09:51:36 2026 -0700  Kevin Moore  fix(js_interop_gen): update js_type_supertypes for Dart 3.12 (dart-lang/web#582)

Change-Id: I4532049c45561fe8f8c57713ac8ec2153096fd9a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/560660
Commit-Queue: Nate Bosch <[email protected]>
Reviewed-by: Brian Quinlan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liamappelbe liamappelbe liamappelbe approved these changes

Assignees

No one assigned

Labels

package:ffigen

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[ffigen] Support passing C++ classes by value

2 participants

@Manar-Elhabbal7 @liamappelbe