-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[pigeon] Improve C++ data class constructors #3585
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
[pigeon] Improve C++ data class constructors #3585
Conversation
tarrinneal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. A couple very minor and optional nits.
|
|
||
| indent.format(''' | ||
| ${klass.name}::${klass.name}($paramString)$initializerString {} | ||
| '''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this could be done with a non-format method that would be easier to maintain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I thought at first too, but this was actually the best option I found. The main issue is that you can't have trailing commas in parameter lists or initializer lists in C++, which means that the options are either join (as I've done here, which then requires format), or an indexed for loop with a conditional on each line to add or not add a comma at the end, and that's just much messier.
And then it gets even uglier if you want to make the empty version:
Foo::Foo() {}
rather than
Foo::Foo(
) {}
because you have to duplicate the entire statement in a conditional, one printing the complete line with no params, and one printing only up to the (, then doing the for loop, then doing the ) [...] {}.
I don't love this version, but I liked the other way a lot less when I initially tried it.
(Longer term, I would like to make a helper method for the C++ generator for printing function declarations and definitions, and refactoring everything to use it, to reduce the need for this kind of thing inline.)
[pigeon] Improve C++ data class constructors
Instead of generating a default constructor for C++ data classes that will leave non-nullable POD fields uninitialized (and other non-nullable fields default-initialized, which doesn't match what we do for other languages), this creates either one or two initializers:
Other changes that followed from this:
Tthat became an error once the default constructor for data classes went away.Fixes flutter/flutter#104653
Fixes flutter/flutter#104286
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.///).