-
Notifications
You must be signed in to change notification settings - Fork 27.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
Make hashCode functions better hashes and less boilerplate #88
Comments
Comment by Hixie From @sethladd on June 12, 2015 4:19 Cc @cbracken fyi
|
Comment by Hixie From @johnmccutchan on June 12, 2015 14:16 https://codereview.chromium.org/1184753002/ Blocked on getting a mirror of quiver setup, which I pinged Robert about. |
Comment by eseidelGoogle We should no longer be blocked on mirrors. We just use pub get to get all our packages which happen w/o mirroring, no? @abarth |
Comment by abarth Correct. |
I looked into this a bit. I agree that we shouldn't be writing our own hashCode functions by hand. I'm not convinced quiver is really the way to go, though. We should do some performance tests to see how it goes. I guess if the VM inlines the relevant functions heavily it's probably ok, but at first blush the functions involve a lot of nested calls and it's not obvious that the performance would be good. What we might want to do instead is build some efficient hashCode functions into dart:ui and use those uniformly everywhere. |
(We should totally use quiver's "areEqualityGroups" to test all our hashCode/operator== functions, though: https://www.dartdocs.org/documentation/quiver/0.21.4/quiver.testing.equality/areEqualityGroups.html ) |
Alternatively/additionnaly you can use source_gen to generate a class/mixin that contains @EqualsAndHashCode()
class FooBar extends _$FooBarEqualsAndHashCode {
FooBar({this.foo, this.bar});
final String foo;
final String bar;
}
// generated in part file
abstract class _$FooBarEqualsAndHashCode {
String get foo;
String get bar;
bool operator ==(o) => identical(this, o) ||
o.runtimeType == runtimeType && o.foo == foo && o.bar == bar;
@override int get hashCode => quiver.hashObjects([foo, bar]); // or implements your hashCode without quiver
} I use something similar in zengen. |
Any resolution here? Or shall we close this one? |
I'm not sure it's worth adding a code generation step. That introduces complexity in the rest of our workflow. We can close the issue if you want. I think it might be better to rename the issue to something like "overriding operator== has too much boilerplate". |
well we definitely need a better way to do hashcodes even if it's not with quiver. |
@abarth How would you feel about a function with the following signature in
The semantics would be that it aggregates the hashCodes of each argument in numeric order up to the first one that's equal to hashEnd (and all the others would be asserted to be unspecified, and asserted to be null), and then aggregates the values in the list, if any (some of our hashCode functions have an N M form where there's a fixed N values and a variable M values in a list). arg1 and arg2 would be required. The function would require that one of the arguments was hashEnd, so if we have 20 arguments, that's up to N=19 values. The hashEnd sentinel value is needed rather than using null for this purpose because sometimes the values we're hashing are null. (The issue of which hash function to use to aggregate the hashCodes from all the values is a separate issue that I'm not trying to address here, but this would make it trivial to test the entire system with various alternatives if we care enough to go study that later.) |
Why not have |
Oh, excellent idea. Especially if we make the list call check for null and handle that specially, so the callers can just pass null lists in without trouble. So something like: final Object hashEnd = new Object();
int hashValues(Object arg1, Object arg2, Object arg3, [ Object arg4, Object arg5,
Object arg6, Object arg7, Object arg8, Object arg9, Object arg10, Object arg11,
Object arg12, Object arg13, Object arg14, Object arg15, Object arg16, Object arg17,
Object arg18, Object arg19, Object arg20 ]);
int hashList(Iterable<Object> args); You'd use it as (this is for LinearGradient): int get hashCode {
return hashValues(begin, end, tileMode, hashList(colors), hashList(stops), hashEnd);
} (Today that function body is 16 lines long.) |
Yeah. |
Another pattern is to use an final Object _undefined = new Object();
int hashValues(Object arg1, Object arg2, [Object arg3 = _undefined,
Object arg4 = _undefined, Object arg5 = _undefined, Object arg6 = _undefined,
Object arg7 = _undefined, Object arg8 = _undefined, Object arg9 = _undefined,
Object arg10 = _undefined, Object arg11 = _undefined,
Object arg12 = _undefined, Object arg13 = _undefined,
Object arg14 = _undefined, Object arg15 = _undefined,
Object arg16 = _undefined, Object arg17 = _undefined,
Object arg18 = _undefined, Object arg19 = _undefined,
Object arg20 = _undefined]) {
//...
} |
Oh, yeah, making the sentinel be the default value of these arguments is a much better solution. |
Thank you very much! |
This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of |
Issue by Hixie
Thursday Jul 16, 2015 at 22:44 GMT
Originally opened as https://github.com/flutter/engine/issues/62
From @eseidelGoogle on June 12, 2015 1:28
http://www.dartdocs.org/documentation/quiver/0.21.4/index.html#quiver/quiver-core
It's silly we write our own hashCodes by hand.
We'll need to add quiver to DEPS.
@johnmccutchan
Copied from original issue: domokit/mojo#236
The text was updated successfully, but these errors were encountered: