-
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
[pigeon] Implement equals for Java data classes #6992
[pigeon] Implement equals for Java data classes #6992
Conversation
@@ -168,7 +179,7 @@ public void hasValues() { | |||
.setBoolList(Arrays.asList(new Boolean[] {true, false})) | |||
.setDoubleList(Arrays.asList(new Double[] {0.5, 0.25, 1.5, 1.25})) | |||
.setIntList(Arrays.asList(new Long[] {1l, 2l, 3l, 4l})) | |||
.setList(Arrays.asList(new int[] {1, 2, 3, 4})) | |||
.setList(Arrays.asList(genericList)) |
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.
This change is because an inherent limitation in Java equals
implementations is that List
equality only works if each element in the list has a useful implementation of equals, and (as demonstrated in the new generator code) arrays don't have that. So having the generic list's value be a nested array makes equality not work. Fixing that would require a bunch of special code generation—we would have to do custom equality checks of all collections to call out to Arrays.equals
for certain types—for a case that I'm skeptical will ever matter in practice, and is a common behavior of the language anyway.
Since the specific type here didn't actually matter, I opted for just changing to something with a useful equals
so that the test will pass.
Looks like you need to gen/format a file |
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.
a questionable nit, make the gen code cleaner, but the generator messier?
__pigeon_result = 31 * __pigeon_result + Arrays.hashCode(aByteArray); | ||
__pigeon_result = 31 * __pigeon_result + Arrays.hashCode(a4ByteArray); | ||
__pigeon_result = 31 * __pigeon_result + Arrays.hashCode(a8ByteArray); | ||
__pigeon_result = 31 * __pigeon_result + Arrays.hashCode(aFloatArray); |
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.
Should we make this a helper?
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.
Doing it inline like this seems to be the stand way I've seen it in other Java code. We'd still need the assignment, so we'd be adding a function call just to abstract away 31 * __pigeon_result +
which seems like a lot of overhead.
flutter/packages@03f5f6d...412ec46 2024-06-28 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.6 to 3.25.10 (flutter/packages#6924) 2024-06-27 ditman@gmail.com [video_player] Exposes VideoPlayerWebOptions. (flutter/packages#6990) 2024-06-27 ditman@gmail.com [ci] Add Wasm compilation to all_packages web app. (flutter/packages#6981) 2024-06-27 10687576+bparrishMines@users.noreply.github.com [interactive_media_ads] Updates `README` with a usage section and fix some interface docs (flutter/packages#6988) 2024-06-27 stuartmorgan@google.com [tools] Fix vm test requirement (flutter/packages#6995) 2024-06-27 jacksongardner@google.com Update `web_benchmarks` package to properly support wasm. (flutter/packages#6970) 2024-06-27 stuartmorgan@google.com [pigeon] Implement equals for Java data classes (flutter/packages#6992) 2024-06-25 matanlurey@users.noreply.github.com Final refactor of `video_player_android` before `SurfaceProducer#setCallback`. (flutter/packages#6982) 2024-06-25 stuartmorgan@google.com [pigeon] Update testing and docs (flutter/packages#6984) 2024-06-25 parlough@gmail.com [various] Update flutter.dev links to more reliable destinations (flutter/packages#6963) 2024-06-25 10687576+bparrishMines@users.noreply.github.com [interactive_media_ads] Adds initial Android implementation (flutter/packages#6733) 2024-06-25 engine-flutter-autoroll@skia.org Roll Flutter from 6c06abb to e726eb4 (51 revisions) (flutter/packages#6987) 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
flutter/packages@03f5f6d...412ec46 2024-06-28 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.6 to 3.25.10 (flutter/packages#6924) 2024-06-27 ditman@gmail.com [video_player] Exposes VideoPlayerWebOptions. (flutter/packages#6990) 2024-06-27 ditman@gmail.com [ci] Add Wasm compilation to all_packages web app. (flutter/packages#6981) 2024-06-27 10687576+bparrishMines@users.noreply.github.com [interactive_media_ads] Updates `README` with a usage section and fix some interface docs (flutter/packages#6988) 2024-06-27 stuartmorgan@google.com [tools] Fix vm test requirement (flutter/packages#6995) 2024-06-27 jacksongardner@google.com Update `web_benchmarks` package to properly support wasm. (flutter/packages#6970) 2024-06-27 stuartmorgan@google.com [pigeon] Implement equals for Java data classes (flutter/packages#6992) 2024-06-25 matanlurey@users.noreply.github.com Final refactor of `video_player_android` before `SurfaceProducer#setCallback`. (flutter/packages#6982) 2024-06-25 stuartmorgan@google.com [pigeon] Update testing and docs (flutter/packages#6984) 2024-06-25 parlough@gmail.com [various] Update flutter.dev links to more reliable destinations (flutter/packages#6963) 2024-06-25 10687576+bparrishMines@users.noreply.github.com [interactive_media_ads] Adds initial Android implementation (flutter/packages#6733) 2024-06-25 engine-flutter-autoroll@skia.org Roll Flutter from 6c06abb to e726eb4 (51 revisions) (flutter/packages#6987) 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
Adds implementations of
equals
andhashCode
to Java data classes. This is frequently useful for native unit tests of plugins using Pigeon (e.g., when using a mock FlutterApi implementation to check that the expected call is being made with the right arguments).Part of flutter/flutter#118087
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, or this PR is exempt from CHANGELOG changes.///
).