-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Fix unnecessary toList/fromList calls during encode/decode process #6426
Conversation
Per offline discussion I think we should adjust the PR description here, as well as the changelog. It's not that things were double-encoding custom classes, it's that we were making the encoding logic needlessly complex by calling ToList/FromList directly instead of letting the encoder do the encoding internally. (This PR will actually make the byte encoding slightly less efficient; before a custom class was encoded as |
@@ -186,15 +186,15 @@ ArrayList<Object> toList() { | |||
return toListResult; | |||
} | |||
|
|||
static @NonNull MessageData fromList(@NonNull ArrayList<Object> list) { | |||
static @NonNull MessageData fromList(@NonNull ArrayList<Object> __pigeon_list) { |
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.
Doesn't need to be in this PR, but we should consider just inlining all of the value expressions into the setter calls so we don't need variables, and thus don't have a collision concern.
@@ -3,6 +3,7 @@ | |||
// found in the LICENSE file. | |||
// Autogenerated from Pigeon, do not edit directly. | |||
// See also: https://pub.dev/packages/pigeon | |||
@file:Suppress("UNCHECKED_CAST", "LocalVariableName", "ArrayInDataClass") |
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.
Why the change? Don't we want to make the suppressions for this as targeted as possible?
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.
most of them are full file regardless, it gives "redundant suppression" errors.
Now I remember why this error looked familiar: it's a bug in VS 2019, which was fixed in VS 2022. We actually shouldn't still be using VS 2019 in CI, but it's going to be tricky to update that right now so I'll dig up the workaround. IIRC it will just require turning on exceptions for the test app. |
Seems like it may be something else still... |
Or it is that same problem, but I put the workaround on the example app binary when the failure is coming from a unit test file that's not compiled in that binary, and I need to change the build settings of the thing that was actually failing to compile 🤦🏻 |
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! Just a few minor comments about possible simplifications.
…lutter#6426) Fix unnecessary toList/fromList calls during encode/decode process. also makes some kotlin code more idiomatic. removes the ability to have collisions with the name `list`. fixes flutter/flutter#119351
flutter/packages@f4719ca...2dfe645 2024-05-06 PROGrand@users.noreply.github.com [camera] MediaSettings parameter for createCameraWithSettings (flutter/packages#3586) 2024-05-06 tarrinneal@gmail.com Fix unnecessary toList/fromList calls during encode/decode process (flutter/packages#6426) 2024-05-03 engine-flutter-autoroll@skia.org Roll Flutter from bf7191f to f1037a0 (21 revisions) (flutter/packages#6641) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC flutter-ecosystem@google.com,rmistry@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…lutter#6426) Fix unnecessary toList/fromList calls during encode/decode process. also makes some kotlin code more idiomatic. removes the ability to have collisions with the name `list`. fixes flutter/flutter#119351
…lutter#6426) Fix unnecessary toList/fromList calls during encode/decode process. also makes some kotlin code more idiomatic. removes the ability to have collisions with the name `list`. fixes flutter/flutter#119351
Fix unnecessary toList/fromList calls during encode/decode process.
also makes some kotlin code more idiomatic.
removes the ability to have collisions with the name
list
.fixes flutter/flutter#119351
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to 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.