[pigeon] add support for top level consts#12032
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 adds support for generating top-level constants across all supported platforms in Pigeon. Feedback on the changes suggests resolving compilation issues by avoiding the escaping of the $ character in the shared double-quote escaping helper and instead handling it specifically in Kotlin. Additionally, double constants initialized with integer literals should be explicitly parsed as doubles to prevent type mismatch errors in Java and Kotlin, and Java constants should be generated as primitive types rather than boxed types to ensure they function as true compile-time constants.
|
/gemini |
|
/gemini |
|
|
||
| NS_ASSUME_NONNULL_BEGIN | ||
|
|
||
| static NSString *const PGNaStringConstant = @"stringConstantValue"; |
There was a problem hiding this comment.
Because of the prefixing, the variable name needs to be converted to UpperCamelCase. E.g., PGNAStringConstant.
| return _makeDartStringLiteral(value.toString()); | ||
| } else { | ||
| return value.toString(); | ||
| } |
There was a problem hiding this comment.
Nit: Consider a single return statement with a ternary.
|
|
||
| String _makeDartStringLiteral(String valStr) { | ||
| final bool hasSpecial = | ||
| // ignore: use_raw_strings |
There was a problem hiding this comment.
Why not just use a raw string for this one?
| if (!valStr.contains('"')) { | ||
| return '"$valStr"'; | ||
| } | ||
| return "r'''$valStr'''"; |
There was a problem hiding this comment.
What if the string contains '''s? I think you'll need to make the fallback escaping ' instead of using '''.
| return '${value}L'; | ||
| } else { | ||
| return value.toString(); | ||
| } |
There was a problem hiding this comment.
Nit: consider return switch (type) {...}
|
|
||
| NS_ASSUME_NONNULL_BEGIN | ||
|
|
||
| static NSString *const PGNaStringConstant = @"stringConstantValue"; |
There was a problem hiding this comment.
Consts should not be inlined into headers in C. These should be extern declarations, and the definitions should be in the .g.m file.
| /// Escapes special characters in a string for use in double-quoted string literals. | ||
| String escapeStringDoubleQuotes(String value) { | ||
| return value | ||
| .replaceAll('\\', r'\\') // ignore: use_raw_strings |
There was a problem hiding this comment.
Here and below, why not just use raw strings?
| ), | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
This should be able to shrink down to:
Map allowedValueTypes = <String, Type>{
'String': String,
'int': int,
'double': num,
'bool': bool,
};
for (final Constant constant in _constants) {
final String typeName = constant.type.baseName;
final Type? expectedValueType = allowedValueTypes[typeName];
if (expectedValueType == null) {
... (existing error message from what is currently a 4-condition if)
}
if (constant.value is! expectedValueType) {
...
message: 'Constant ${constant.name}" type is ${typeName} but value type is ${constant.value.runtimeType}.'
}
}(This is freehand in the text field so there may be minor mistakes, but I think it's pretty much correct.)
| _errors.add( | ||
| Error( | ||
| message: | ||
| 'Unsupported expression type ${expression.runtimeType} for constant initializer.', |
There was a problem hiding this comment.
We may get requests for bit-shift operator support, but we can cross that bridge if we come to it.
| import 'package:pigeon/pigeon.dart'; | ||
|
|
||
| const String aStringConstant = 'stringConstantValue'; | ||
| const String aStringConstantWithEscapes = r'string\\$ConstantValue'; |
There was a problem hiding this comment.
Shouldn't ' and " be included in this string as well?
2 participants
Add Top-level Constants Support to Pigeon
This PR adds support for defining and generating top-level constants across all target languages supported by Pigeon (Dart, Kotlin, Java, Swift, Objective-C, C++, and GObject).
Resolves: flutter/flutter#188642
Summary of Changes
1. AST & Parser
Constantnode type to the AST representing top-level constants.pigeon_lib_internal.dartto parseconstdeclarations.String,int,double, andbool.2. Code Generation
Added constant generation to all target languages:
constvariables.public static finalfields inside the main generated class.const valproperties (usingvalfor double/boolean where JVMconstis not supported).public letconstants.static const/static NSString *constvariables in the header file.inline constexprvariables in the header.#definemacros in the header.3. Tests & Documentation
test/pigeon_lib_test.dart.test/*_generator_test.dart) for each language verifying the syntax of generated constants.pigeons/core_tests.dartwith constant declarations, and added corresponding unit tests verifying their correctness in each target platform project.platform_tests/shared_test_plugin_code/lib/integration_tests.dartto assert constant values at runtime.packages/pigeon/README.mdto document the new feature, using code excerpts linked toexample/app/pigeons/messages.dart.