ignore #10232
ignore #10232
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a substantial new feature for JNI and FFI support in Pigeon, alongside various formatting and refactoring changes across the codebase. The core of the changes lies in the Dart, Swift, and Kotlin generators. My review has focused on identifying critical issues and potential bugs. I've found hardcoded local paths in test configuration files that must be addressed, as well as an empty file that appears to be an accidental addition. Additionally, I've identified a potential bug in the Dart generator's handling of FFI enums. The remaining changes, including formatting and refactoring, seem reasonable and consistent with the new feature implementation.
| ObjCCompatibleSwiftFileInput( | ||
| files: <Uri>[ | ||
| Uri.file( | ||
| '/Users/tarrinneal/work/packages/packages/pigeon/platform_tests/test_plugin/darwin/test_plugin/Sources/test_plugin/NiTests.gen.swift', |
There was a problem hiding this comment.
This file contains a hardcoded absolute path, which includes a local username (/Users/tarrinneal/...). This will cause the build to fail for other developers. Please replace this with a relative path or ensure this file is generated dynamically and not checked into source control.
| @@ -0,0 +1,25 @@ | |||
| android_sdk_config: | |||
| add_gradle_deps: true | |||
| android_example: '/Users/tarrinneal/work/packages/packages/pigeon/platform_tests/test_plugin/example' | |||
There was a problem hiding this comment.
This configuration file contains a hardcoded absolute path with a local username (/Users/tarrinneal/...). This will break the build on other machines. This path should be made relative or the file should be generated dynamically during the build process and not be part of the source control.
| if (type.isEnum && !type.isNullable) { | ||
| return 'ffi_bridge.${type.baseName}.values[$name]'; | ||
| } |
There was a problem hiding this comment.
This special handling for non-nullable enums appears to be incorrect. The generated code ffi_bridge.${type.baseName}.values[$name] attempts to access a Dart enum from an FFI bridge file, which won't work as the bridge file contains FFI bindings, not Dart code.
The native function expects an integer representation for the enum. The name variable already holds the correct integer value (myField.index). The fallback logic using _PigeonFfiCodec.writeValue correctly handles converting an integer to an NSNumber, which is the expected behavior.
Removing this block will allow the general _PigeonFfiCodec.writeValue to handle non-nullable enums correctly, consistent with how nullable enums are handled.
| @@ -0,0 +1 @@ | |||
|
|
|||
There was a problem hiding this comment.
This file is empty. It seems to have been added by mistake and should probably be removed.
1 participant
No description provided.