Repository navigation
[pigeon] Implements primitive enums for arguments to HostApis - #1871
Conversation
gaaclarke
changed the title
There was a problem hiding this comment.
Should this be called argValues or argValueExpressions or something like that? foo.index isn't the name of an argument.
There was a problem hiding this comment.
Needs a declaration comment.
There was a problem hiding this comment.
Doesn't an enum need to be wrapped in an NSNumber so that it can be nullable, like other primitives? (And shouldn't that logic be in _objcTypeForDartType?)
There was a problem hiding this comment.
We are blocking nullable enums for now so it isn't necessary. When we support nullable enums that would be a good way to implement them.
There was a problem hiding this comment.
Sorry, I hadn't fully paged this back in when I re-reviewed. Even though you aren't supporting nullable enums now, we will want to add that support soon (since it's a weird gap), so it should be an NSNumber now otherwise it will be a breaking change to add nullable support.
There was a problem hiding this comment.
I was thinking I'd use nullable NSNumber* for nullable enums and the primitive type for non-null enums. That way the usage is cleaner, safer and it matches the behavior we have for enums in class fields. I don't think nullable enums are that high priority anyways since the null value can easily be represented with an enum field. That would be preferable to losing the type safety of dealing directly with enums versus introducing nsnumbers.
There was a problem hiding this comment.
I didn't realize that's what we were doing for enum class fields, I thought they were boxed as well. The inconsistency with other primitive types, which we box regardless of whether or not they are nullable, seems odd, but it sounds like that ship has already sailed. Maybe at some point we can do a breaking change to better unify handling if we start doing switching on enums.
There was a problem hiding this comment.
Unless I'm misunderstanding the code this generates, this would convert a null value from dart to whatever enum value zero is.
There was a problem hiding this comment.
Yep, but we are blocking nullable enums right now.
There was a problem hiding this comment.
I guess this answers some of my questions above. But couldn't we just support it instead?
There was a problem hiding this comment.
I'm just cutting off scope to what I need for now. I don't have a lot of time to open this up.
There was a problem hiding this comment.
Okay, this should be updated in the README though (https://github.com/flutter/packages/blob/main/packages/pigeon/README.md#null-safety-nnbd)
|
@stuartmorgan a few of your comments are good points but I'm pretty sure they are relevant only if we support nullable enum arguments which I didn't do here. PTAL. |
There was a problem hiding this comment.
Okay, this should be updated in the README though (https://github.com/flutter/packages/blob/main/packages/pigeon/README.md#null-safety-nnbd)
There was a problem hiding this comment.
You'll need to block this in the C++ generator as part of this PR, now that C++ has landed.
| // TODO(gaaclarke): Add line number and filename. | ||
| result.add(Error( | ||
| message: | ||
| 'Nullable enum types aren\'t support in C++ arguments in method:${api.name}.${method.name} argument:(${arg.type.baseName} ${arg.name}).')); |
There was a problem hiding this comment.
Sorry, I hadn't fully paged this back in when I re-reviewed. Even though you aren't supporting nullable enums now, we will want to add that support soon (since it's a weird gap), so it should be an NSNumber now otherwise it will be a breaking change to add nullable support.
3 participants
issue: flutter/flutter#87307
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.