Make hashCode functions better hashes and less boilerplate

@eseidelGoogle

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

@sethladd

Comment by Hixie
Thursday Jul 16, 2015 at 22:45 GMT

From @sethladd on June 12, 2015 4:19

Cc @cbracken fyi
On Fri, Jun 12, 2015 at 03:28 Eric Seidel [email protected] wrote:

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 https://github.com/johnmccutchan

—
Reply to this email directly or view it on GitHub
https://github.com/domokit/mojo/issues/236.

@johnmccutchan

Comment by Hixie
Thursday Jul 16, 2015 at 22:45 GMT

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.

@abarth

Comment by eseidelGoogle
Friday Aug 14, 2015 at 20:29 GMT

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
Friday Aug 14, 2015 at 21:44 GMT

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 hashCode and operator ==.

@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

@abarth How would you feel about a function with the following signature in dart:ui?

final Object hashEnd = new Object();
int aggregateHash({ 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, Iterable<Object> argList });

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 aggregateHash take optional parameters (rather than named parameters) and then do the Iterable<Object> in another function call? The code that has the N+M structure would then make two calls, which would hopefully be uncommon.

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 _undefined const as default parameter. It will allow to avoid the use of hashEnd everywhere.

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 flutter doctor -v and a minimal reproduction of the issue.

Navigation Menu

Repository navigation

Make hashCode functions better hashes and less boilerplate #88

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Make hashCode functions better hashes and less boilerplate #88

Description

Activity

FlutterIssues commented on Nov 9, 2015

FlutterIssues commented on Nov 9, 2015

FlutterIssues commented on Nov 9, 2015

FlutterIssues commented on Nov 9, 2015

Hixie commented on Dec 8, 2015

Hixie commented on Dec 8, 2015

a14n commented on Dec 8, 2015

sethladd commented on Dec 8, 2015

abarth commented on Dec 9, 2015

Hixie commented on Dec 9, 2015

Hixie commented on Dec 9, 2015

abarth commented on Dec 9, 2015

Hixie commented on Dec 9, 2015

abarth commented on Dec 9, 2015

a14n commented on Dec 9, 2015

Hixie commented on Dec 9, 2015

eseidelGoogle commented on Dec 10, 2015

github-actions commented on Sep 6, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions