Skip to content

More quick fixes for bugs related to #1250 #1339

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

Merged
merged 7 commits into from
Jul 17, 2024
Merged

Conversation

liamappelbe
Copy link
Contributor

Fixes some more small bugs in the generated ObjC code related to listener blocks:

  • Resolve naming conflicts in the ObjC wrapper functions, by adding infra to run the UniqueNamer on certain Binding.originalNames.
  • Remove the _Nullable annotation from the generated ObjC code. It doesn't really do anything in our use case, and is an invalid annotation on certain types.
  • Use #import instead of #include
  • Use #import <Framework/Header.h> style imports instead of #import "../../../Relative/Path/To/Framework/Header.h" for headers that are part of a framework.

Fixes #1275

Also remove nullable annotation from native code. It doesn't do
anything and is sometimes invalid.
Copy link

github-actions bot commented Jul 15, 2024

PR Health

Coverage ⚠️

Details
File Coverage
pkgs/ffigen/lib/src/code_generator/func.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/objc_block.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/objc_nullable.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/utils.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/writer.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 ✔️

Details
// 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/lang/jcharacter.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/swiftgen/swift2objc/lib/src/generator/_core/utils.dart
pkgs/swiftgen/swift2objc/lib/src/generator/generator.dart
pkgs/swiftgen/swift2objc/lib/src/generator/generators/class_generator.dart
pkgs/swiftgen/swift2objc/lib/src/transformer/_core/unique_namer.dart

Package publish validation ✔️

Details
Package Version Status
package:ffi 2.1.3-wip WIP (no publish necessary)
package:ffigen 13.0.0-wip WIP (no publish necessary)
package:jni 0.10.1 already published at pub.dev
package:jnigen 0.10.0 already published at pub.dev
package:native_assets_cli 0.7.2 already published at pub.dev
package:objective_c 1.1.0-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.

@coveralls
Copy link

coveralls commented Jul 15, 2024

Coverage Status

coverage: 90.735% (-1.3%) from 92.037%
when pulling 7085046 on morelittlefixes
into 3bc5454 on main.

@liamappelbe liamappelbe marked this pull request as ready for review July 16, 2024 02:07
@liamappelbe liamappelbe requested a review from dcharkes July 16, 2024 02:07
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.

Resolve naming conflicts in the ObjC wrapper functions, by adding infra to run the UniqueNamer on certain Binding.originalNames.

I think we should maybe introduce a third concept besides originalName and name.

@@ -275,6 +275,7 @@ $blockTypedef $fnName($blockTypedef block) {
objCReturnsRetained: true,
isLeaf: true,
isInternal: true,
originalNameIsRenamable: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this is the right approach.

Conceptually any original names (from the source language) should not be renameable, because looking up things would fail.

Here we actually bind to some native code that we generate, so I would maybe not call that the original name.

Is there a way we can separate the concepts of (1) a name in the native source code "originalName" and (2) a name in the native source code that we generate "intermediateName". (And then "name" for what the user-uses.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'd need a flag in Func to tell it whether to use the originalName or intermediateName when looking up the symbol. When considering that option I realized we can just add a flag that tells the Func to use the name instead of the originalName for lookup, and then we wouldn't need any extra renaming steps in the library.

@liamappelbe liamappelbe merged commit 281124f into main Jul 17, 2024
17 checks passed
@liamappelbe liamappelbe deleted the morelittlefixes branch July 17, 2024 02:38
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.

Objective-C generated code results in module redefinition
3 participants