[pigeon] swift equality methods #8971
Conversation
| return true | ||
|
|
||
| case let (cleanLhsHashable, cleanRhsHashable) as (AnyHashable, AnyHashable): | ||
| return cleanLhsHashable == cleanRhsHashable |
There was a problem hiding this comment.
Hashable doesnt mean equal
There was a problem hiding this comment.
hashable inherits from equitable, there is no AnyEquitable.
There was a problem hiding this comment.
sorry i misread. i thought you were comparing 2 hash values.
| } | ||
| } | ||
| return true | ||
|
|
There was a problem hiding this comment.
can you also check the case when both conform to Equatable, then use == to compare them? (e.g. String is Equatable)
There was a problem hiding this comment.
String is Hashable, therefore already covered. Any type that is supported by pigeon is covered by hashable except lists and maps. And certain classes in proxy apis, but this isn't meant to solve for non-Hashable classes, as there is no real way to compare if they aren't Equitable.
There was a problem hiding this comment.
Sorry a String isn't a good example. Worth putting a comment there to explain a bit, since this is missing Non Hashable but Equatable case.
| return true | ||
|
|
||
| default: | ||
| return String(describing: cleanLhs) == String(describing: cleanRhs) |
There was a problem hiding this comment.
where does the input come from? one could overwrite a type's description to return arbitrary strings.
There was a problem hiding this comment.
yeah, I couldn't find a better way to manage this besides just returning false. This seemed potentially better? This isn't (theoretically) hittable anyway since pigeon types are limited
There was a problem hiding this comment.
when will we hit the default case? we might as well just return false if we already covered all known cases that return true. Also worth adding a comment.
...ple/app/android/app/src/main/kotlin/dev/flutter/pigeon_example_app/EventChannelMessages.g.kt
Outdated
Show resolved
Hide resolved
| description == other.description && | ||
| code == other.code && | ||
| deepEqualsMessages(data, other.data) | ||
| return deepEqualsMessages(toList(), other.toList()) |
There was a problem hiding this comment.
FWIW this is making the generated code shorter at the cost of making the method less efficient, since now it has to create two new arrays.
I don't think the efficiency matters much, but worth noting that there is a tradeoff, it's not just shorter.
| case let (cleanLhsDictionary, cleanRhsDictionary) as ([AnyHashable: Any?], [AnyHashable: Any?]): | ||
| guard cleanLhsDictionary.count == cleanRhsDictionary.count else { return false } | ||
| for (key, cleanLhsValue) in cleanLhsDictionary { | ||
| guard let cleanRhsValue = cleanRhsDictionary[key] else { return false } |
There was a problem hiding this comment.
Won't this give the wrong answer if two equal dictionaries have a nil value for some key?
There was a problem hiding this comment.
we don't support nil keys
There was a problem hiding this comment.
Not nil keys, nil values. guard let cleanRhsValue = cleanRhsDictionary[key] else { return false } will unconditionally return false for "foo": nil even if both dictionaries have it.
There was a problem hiding this comment.
ah, misread, my bad
There was a problem hiding this comment.
There is no solution to this problem that doesn't include creating a list of keys
| } | ||
|
|
||
| if let valueDict = value as? [AnyHashable: AnyHashable] { | ||
| for key in valueDict.keys { |
There was a problem hiding this comment.
for (key, value) in valueDict {As with the Kotlin comment: if you need keys and values, an iterator that keeps both is more efficient than iterating keys and then looking up the value.
There was a problem hiding this comment.
This is already in this shape, not sure why it's not showing in your review
tarrinneal
added
the
autosubmit
flutter/packages@267ac7b...2405f6a 2025-04-08 [email protected] [pigeon] swift equality methods (flutter/packages#8971) 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 [email protected] 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 == and hash methods to classes
adds == and hash methods to classes
adds == and hash methods to classes
flutter/packages@267ac7b...2405f6a 2025-04-08 [email protected] [pigeon] swift equality methods (flutter/packages#8971) 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 [email protected] 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
3 participants
adds == and hash methods to classes