Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ffigen] Return structs by value from ObjC methods #1556

Merged
merged 11 commits into from
Sep 30, 2024
Merged

[ffigen] Return structs by value from ObjC methods #1556

merged 11 commits into from
Sep 30, 2024

Conversation

liamappelbe
Copy link
Contributor

@liamappelbe liamappelbe commented Sep 13, 2024

Returns structs by value from ObjC methods by allocating their backing storage using calloc, converting that pointer to a TypedData with calloc.nativeFree as its finalizer, then converting that to the struct using Struct.create. For example, these are some bindings from method_test.dart:

/// twiddleVec4Components:
Vec4 twiddleVec4Components_(Vec4 v) {
  final _ptr = pkg_ffi.calloc<Vec4>();
  final _data = _ptr
      .cast<ffi.Uint8>()
      .asTypedList(ffi.sizeOf<Vec4>(), finalizer: pkg_ffi.calloc.nativeFree);
  objc.useMsgSendVariants
      ? _objc_msgSend_5Stret(
          _ptr, this.ref.pointer, _sel_twiddleVec4Components_, v)
      : _ptr.ref =
          _objc_msgSend_5(this.ref.pointer, _sel_twiddleVec4Components_, v);
  return ffi.Struct.create<Vec4>(_data);
}

Fixes #1480

Copy link

github-actions bot commented Sep 13, 2024

PR Health

Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

Coverage ⚠️
File Coverage
pkgs/ffigen/lib/src/code_generator/objc_interface.dart 💔 Not covered

This check for test coverage is informational (issues shown here will not fail the PR).

This check can be disabled by tagging the PR with skip-coverage-check.

License Headers ✔️
// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers
Files
pkgs/ffigen/example/libclang-example/generated_bindings.dart
pkgs/ffigen/example/shared_bindings/generate.dart
pkgs/ffigen/example/shared_bindings/lib/generated/a_gen.dart
pkgs/ffigen/example/shared_bindings/lib/generated/a_shared_b_gen.dart
pkgs/ffigen/example/shared_bindings/lib/generated/base_gen.dart
pkgs/ffigen/example/simple/generated_bindings.dart
pkgs/ffigen/lib/src/header_parser/clang_bindings/clang_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_decl_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_symbol_address_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_type_name_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_reserved_keyword_collision_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_comment_markup_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_dart_handle_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_enum_int_mimic_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_forward_decl_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_functions_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_imported_types_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_native_func_typedef_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_opaque_dependencies_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_packed_structs_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_regress_384_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_struct_fptr_fields_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_typedef_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_unions_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_varargs_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_cjson_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_libclang_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_sqlite_bindings.dart
pkgs/ffigen/test/native_test/_expected_native_test_bindings.dart
pkgs/jni/lib/src/third_party/generated_bindings.dart
pkgs/jni/lib/src/third_party/global_env_extensions.dart
pkgs/jni/lib/src/third_party/jni_bindings_generated.dart
pkgs/jnigen/android_test_runner/lib/main.dart
pkgs/jnigen/example/in_app_java/lib/android_utils.dart
pkgs/jnigen/example/kotlin_plugin/example/lib/main.dart
pkgs/jnigen/example/kotlin_plugin/lib/kotlin_bindings.dart
pkgs/jnigen/example/kotlin_plugin/lib/kotlin_plugin.dart
pkgs/jnigen/example/pdfbox_plugin/lib/pdfbox_plugin.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/PDDocument.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/PDDocumentInformation.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/_package.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/text/PDFTextStripper.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/text/_package.dart
pkgs/jnigen/lib/src/bindings/descriptor.dart
pkgs/jnigen/lib/src/elements/elements.g.dart
pkgs/jnigen/test/jackson_core_test/third_party/bindings/com/fasterxml/jackson/core/_package.dart
pkgs/jnigen/tool/command_runner.dart
pkgs/native_assets_builder/test_data/native_dynamic_linking/bin/native_dynamic_linking.dart
pkgs/swift2objc/lib/src/config.dart
pkgs/swift2objc/lib/src/generate_wrapper.dart
pkgs/swift2objc/lib/src/generator/_core/utils.dart
pkgs/swift2objc/lib/src/generator/generator.dart
pkgs/swift2objc/lib/src/generator/generators/class_generator.dart
pkgs/swift2objc/lib/src/parser/parsers/declaration_parsers/parse_initializer_declaration.dart
pkgs/swift2objc/lib/src/parser/parsers/declaration_parsers/parse_property_declaration.dart
pkgs/swift2objc/lib/src/transformer/_core/unique_namer.dart
pkgs/swift2objc/lib/src/transformer/_core/utils.dart
pkgs/swift2objc/lib/src/transformer/transformers/transform_property.dart
pkgs/swift2objc/test/unit/parse_initializer_param_test.dart
Package publish validation ✔️
Package Version Status
package:ffi 2.1.3 already published at pub.dev
package:ffigen 15.0.0-wip WIP (no publish necessary)
package:jni 0.12.0-wip WIP (no publish necessary)
package:jnigen 0.12.0-wip WIP (no publish necessary)
package:native_assets_cli 0.9.0-wip WIP (no publish necessary)
package:objective_c 2.1.0-wip WIP (no publish necessary)
package:swift2objc 0.0.1-wip WIP (no publish necessary)
package:swiftgen 0.0.1-wip WIP (no publish necessary)

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

@liamappelbe liamappelbe marked this pull request as ready for review September 27, 2024 01:40
@coveralls
Copy link

coveralls commented Sep 27, 2024

Coverage Status

coverage: 92.094% (+0.1%) from 91.993%
when pulling 2d8a282 on stret
into f9a3888 on main.

@dcharkes
Copy link
Collaborator

It should instead be

  final _ptr = pkg_ffi.calloc<Vec4>();
  objc.useMsgSendVariants
      ? _objc_msgSend_5Stret(
          _ptr, this.ref.pointer, _sel_twiddleVec4Components_, v)
      : _ptr.ref =
          _objc_msgSend_5(this.ref.pointer, _sel_twiddleVec4Components_, v);
  final _data = _ptr
      .cast<ffi.Uint8>()
      .asTypedList(ffi.sizeOf<Vec4>(), finalizer: pkg_ffi.calloc.nativeFree);
  return ffi.Struct.create<Vec4>(_data);

Otherwise, if the method is inlined the finalizer will still run before before the native call.

Copy link
Collaborator

@dcharkes dcharkes left a comment

Choose a reason for hiding this comment

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

lgtm once code is reordered to prevent finalizer from running early on inlining.

@liamappelbe liamappelbe merged commit 001bd08 into main Sep 30, 2024
18 checks passed
@liamappelbe liamappelbe deleted the stret branch September 30, 2024 00:48
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.

Obj-C methods returning structs are generated incorrectly
3 participants