[pigeon] implements nullable return types #849
Conversation
gaaclarke
force-pushed
the
nullable-return-types
branch
from
d5e4b22 to
77a4216
Compare
There was a problem hiding this comment.
details isn't required, so this line can just be removed.
There was a problem hiding this comment.
(Same in the existing code just below; where we can trivially make the generated code less verbose we should do so.)
There was a problem hiding this comment.
The fact that this is before the if line in the generating code was super confusing to me in trying to follow this. I think it would be helpful to make each of the if/else if/else blocks a local variable, declared in order, and then use all three below, so that generated code sequence is in order in this code.
There was a problem hiding this comment.
(Longer term than this PR: I think the complexity has gotten to the point where we should very strongly consider using a templating system, like we do for the generated plugin registrants in flutter_tool. We could likely make it much easier to read the to-be-generated code that way.)
There was a problem hiding this comment.
I swapped everything now so it shows up in the correct location now. Something like mustache might help. Its hard to guess the ROI right now though.
There was a problem hiding this comment.
nullCheckIfApplicable? It's not always a null check.
There was a problem hiding this comment.
Could you do a comment like the one above for the fragment that this is responsible for making?
There was a problem hiding this comment.
For future reference: it would be helpful to separate a refactor that makes ~the entire function into a diff from functional changes. I have no idea what actually changed in this code beyond the indentation changes from the inner function extraction.
There was a problem hiding this comment.
Sorry about that. I know you asked for it in the past and I accidentally squashed it.
gaaclarke
force-pushed
the
nullable-return-types
branch
from
77a4216 to
08be1d4
Compare
gaaclarke
force-pushed
the
nullable-return-types
branch
from
d54f02c to
8ecd4df
Compare
| : 'return ($accessor as $returnType$nullTag)$unwrapper$castCall;'; | ||
| // On iOS we can return nil from functions to accommodate error | ||
| // handling. Returning a nil value and not returning an error is an | ||
| // exception. |
There was a problem hiding this comment.
This comment got separated from the code it's about (which is now at line 226).
| return type.isVoid | ||
| ? 'void(^)(NSError *_Nullable)' | ||
| : 'void(^)(${objcType.ptr.trim()}, NSError *_Nullable)'; | ||
| : 'void(^)(${objcType.ptr.trim()}$nullable, NSError *_Nullable)'; |
There was a problem hiding this comment.
Doesn't this have to be nullable in all cases, just like the actual synchronous return value, for the case where there's an error?
| // found in the LICENSE file. | ||
| // | ||
| // Autogenerated from Pigeon (v1.0.15), do not edit directly. | ||
| // Autogenerated from Pigeon (v1.0.18), do not edit directly. |
There was a problem hiding this comment.
This PR is updating the version to 19.
| // found in the LICENSE file. | ||
| // | ||
| // Autogenerated from Pigeon (v1.0.15), do not edit directly. | ||
| // Autogenerated from Pigeon (v1.0.18), do not edit directly. |
There was a problem hiding this comment.
Same here and in other files.
2 participants
fixes flutter/flutter#98452
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.