Skip to content

Conversation

@stuartmorgan-g
Copy link
Collaborator

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:

  • One that has parameters for all non-nullable fields.
  • If there are any nullable fields, one that has parameters for all fields as a connivence constructor. (With the caveat that passing values for nullable values is not super convenient since they are pointer arguments.)

Other changes that followed from this:

  • The deserialization function is now a factory rather than a constructor; this was necessary to avoid a collision in the case of a data class whose only non-nullable field is an EncodableList.
  • Deserialization now uses the constructor for non-nullable arguments. (Technically we could have kept a private default constructor and not changed it, but doing it this way makes it easier to reason about not accidentally leaving anything uninitialized).
    • While touching that code, I simplified the handling on non-nullable values to unconditionally read (vs silently ignoring null or wrong-type values), and adjusted the nullable handling to check for null and then unconditionally read (vs silently treating wrong-type values as null), in line with changes in other languages to be stricter about type casting.
  • ErrorOr's constructors had to be adjusted slightly, as they had a hidden default initialization of T that became an error once the default constructor for data classes went away.

Fixes flutter/flutter#104653
Fixes flutter/flutter#104286

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

Copy link
Contributor

@tarrinneal tarrinneal left a 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 {}
''');
Copy link
Contributor

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.

Copy link
Collaborator Author

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.)

@stuartmorgan-g stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 30, 2023
@auto-submit auto-submit bot merged commit ee37f1c into flutter:main Mar 30, 2023
@stuartmorgan-g stuartmorgan-g deleted the pigeon-cpp-constructors branch March 30, 2023 14:43
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 30, 2023
nploi pushed a commit to nploi/packages that referenced this pull request Jul 16, 2023
[pigeon] Improve C++ data class constructors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[pigeon] C++ generator creates classes with uninitialized fields [pigeon] C++ generator should make convenience constructors

2 participants