Add Object.hashCodeOf to get the default hashCode of an object.

Add Object.hashCodeOf to get the default hashCode of an object.

BUG= http://dartbug.com/13404
R=floitsch@google.com, iposva@google.com

Committed: https://code.google.com/p/dart/source/detail?r=27865

[kasperl, 2013-09-20 13:17:17]

https://codereview.chromium.org/24280003/diff/1/sdk/lib/core/object.dart
File sdk/lib/core/object.dart (right):

https://codereview.chromium.org/24280003/diff/1/sdk/lib/core/object.dart#newc...
sdk/lib/core/object.dart:104: external static int hashCodeOf(Object object);
How about calling this identityHashCode? Maybe consider moving it next to
identical() rather than sticking it in here?

[floitsch, 2013-09-20 13:21:40]

LGTM once we agree on the name.

[Lasse Reichstein Nielsen, 2013-09-20 14:02:40]

I'd prefer identityHashCode (or perhaps even just identityHash) as a top-level
function, but decided to start with the option that didn't put a new name in the
global scope of dart:core.

[sra1, 2013-09-20 14:06:08]

On 2013/09/20 13:17:17, kasperl wrote:
> https://codereview.chromium.org/24280003/diff/1/sdk/lib/core/object.dart
> File sdk/lib/core/object.dart (right):
> 
>
https://codereview.chromium.org/24280003/diff/1/sdk/lib/core/object.dart#newc...
> sdk/lib/core/object.dart:104: external static int hashCodeOf(Object object);
> How about calling this identityHashCode? Maybe consider moving it next to
> identical() rather than sticking it in here?

I think we must change the name, otherwise it is too easily confused with get
hashCode.
The CL title "Add Object.hashCodeOf to get the default hashCode of an object."
hints that 'defaultHashCodeOf' would be a more descriptive name.

[Johnni Winther, 2013-09-20 18:23:02]

https://codereview.chromium.org/24280003/diff/1/sdk/lib/_internal/lib/core_pa...
File sdk/lib/_internal/lib/core_patch.dart (right):

https://codereview.chromium.org/24280003/diff/1/sdk/lib/_internal/lib/core_pa...
sdk/lib/_internal/lib/core_patch.dart:34: // Basic JS non-object types don't
work with objectHashCode.
Use the implementation for _objectHashCode in js_helper.dart instead.

[Lasse Reichstein Nielsen, 2013-09-23 07:08:27]

https://codereview.chromium.org/24280003/diff/1/sdk/lib/_internal/lib/core_pa...
File sdk/lib/_internal/lib/core_patch.dart (right):

https://codereview.chromium.org/24280003/diff/1/sdk/lib/_internal/lib/core_pa...
sdk/lib/_internal/lib/core_patch.dart:34: // Basic JS non-object types don't
work with objectHashCode.
Done. Actually, I made _objectHashCode public in _jshelper, and call it directly
instead of copying the implementation.

I assume the "hashCode" implementation above is ok, because it only affects
non-primitive objects.

[floitsch, 2013-09-23 13:45:13]

I vote for Object.identityHashCodeOf
It is long, but in the end it shouldn't be used that frequently, so having a
long (but descriptive) name shouldn't be a problem.

[Ivan Posva, 2013-09-24 06:21:32]

On 2013/09/23 13:45:13, floitsch wrote:
> I vote for Object.identityHashCodeOf
> It is long, but in the end it shouldn't be used that frequently, so having a
> long (but descriptive) name shouldn't be a problem.

What's wrong with dropping the "Of" and naming it "Object.identityHashCode" or
even identityHashCode at the top-level to sit right next to identical with which
it should be paired?

-Ivan

[Ivan Posva, 2013-09-24 06:22:10]

NMW...

See attached comments.

-Ivan

https://codereview.chromium.org/24280003/diff/11001/runtime/lib/object_patch....
File runtime/lib/object_patch.dart (right):

https://codereview.chromium.org/24280003/diff/11001/runtime/lib/object_patch....
runtime/lib/object_patch.dart:18: // Num doesn't use identity hash for hashCode,
and this method must be
I have read this comment several times and it still does not make much sense to
me. But as noted below it can be entirely avoided.

https://codereview.chromium.org/24280003/diff/11001/runtime/lib/object_patch....
runtime/lib/object_patch.dart:27: static int _hashCodeOf(Object object) {
Please leave this as a private dynamic method on Object and override it where
needed (e.g. _Double, and friends). This will avoid the entirely unnecessary
type checks above as dynamic dispatch will deal with the type appropriately.

https://codereview.chromium.org/24280003/diff/11001/sdk/lib/core/object.dart
File sdk/lib/core/object.dart (right):

https://codereview.chromium.org/24280003/diff/11001/sdk/lib/core/object.dart#...
sdk/lib/core/object.dart:98: * Returns the same value as [hashCode] on [object]
would.
This is not true, as is stated right on the next line.

https://codereview.chromium.org/24280003/diff/11001/sdk/lib/core/object.dart#...
sdk/lib/core/object.dart:102: * hashCode event if `hashCode` has been
overridden.
What this should state is that this returns the hash code which goes together
with the identical function, in a similar manner to hashCode and operator ==.

[kasperl, 2013-09-24 06:24:15]

On 2013/09/24 06:21:32, Ivan Posva wrote:
> On 2013/09/23 13:45:13, floitsch wrote:
> > I vote for Object.identityHashCodeOf
> > It is long, but in the end it shouldn't be used that frequently, so having a
> > long (but descriptive) name shouldn't be a problem.
> 
> What's wrong with dropping the "Of" and naming it "Object.identityHashCode" or
> even identityHashCode at the top-level to sit right next to identical with
which
> it should be paired?
> 
> -Ivan

I also prefer not having the 'Of' suffix.

[Lasse Reichstein Nielsen, 2013-09-24 08:38:57]

My personal favorite is a top-level identityHashCode.
Having it as a static in Object is only done to not reserve a name in the global
scope that will cause conflicts with other uses of the same name.

[Lasse Reichstein Nielsen, 2013-09-24 09:38:56]

Is now top-level identityHashCodeOf.
Will rename to identityHashOf, just for brevity.

Please take a look at VM code again!

[kasperl, 2013-09-24 10:04:06]

On 2013/09/24 09:38:56, Lasse Reichstein Nielsen wrote:
> Is now top-level identityHashCodeOf.
> Will rename to identityHashOf, just for brevity.

To be consistent with hashCode this really needs to be identityHashCode -- not
identityHashOf.

[Lasse Reichstein Nielsen, 2013-09-24 10:28:57]

On 2013/09/24 10:04:06, kasperl wrote:
> On 2013/09/24 09:38:56, Lasse Reichstein Nielsen wrote:
> > Is now top-level identityHashCodeOf.
> > Will rename to identityHashOf, just for brevity.
> 
> To be consistent with hashCode this really needs to be identityHashCode -- not
> identityHashOf.

It can't really be consistent with a getter without being a getter, not the way
we generally name things.

Object.hashCode is a getter, so it has a noun-phrase as name.
The method identityHashCodeOf is not a getter, so it needs a ... well, it's not
a verb-phrase, it should really be "int getIdentityHash(Object o)" for that.
It's just "not-a-noun-phrase".
It's really an "external accessor", which we don't have a naming policy for.

[kasperl, 2013-09-24 10:34:59]

On 2013/09/24 10:28:57, Lasse Reichstein Nielsen wrote:
> On 2013/09/24 10:04:06, kasperl wrote:
> > On 2013/09/24 09:38:56, Lasse Reichstein Nielsen wrote:
> > > Is now top-level identityHashCodeOf.
> > > Will rename to identityHashOf, just for brevity.
> > 
> > To be consistent with hashCode this really needs to be identityHashCode --
not
> > identityHashOf.
> 
> It can't really be consistent with a getter without being a getter, not the
way
> we generally name things.
> 
> Object.hashCode is a getter, so it has a noun-phrase as name.
> The method identityHashCodeOf is not a getter, so it needs a ... well, it's
not
> a verb-phrase, it should really be "int getIdentityHash(Object o)" for that.
> It's just "not-a-noun-phrase".
> It's really an "external accessor", which we don't have a naming policy for.

The consistency comment was about hashCode vs just hash. I still prefer to leave
out the 'Of' suffix too.

[Lasse Reichstein Nielsen, 2013-09-24 19:50:53]

VM implementation changed, PTAL.

[Ivan Posva, 2013-09-24 23:17:20]

On 2013/09/24 19:50:53, Lasse Reichstein Nielsen wrote:
> VM implementation changed, PTAL.

VM implementation LGTM now.

I still believe that this should be a top-level method called identityHashCode
sitting right next to the implementation of identical.

Thanks,
-Ivan

[Lasse Reichstein Nielsen, 2013-09-25 08:15:58]

Committed patchset #5 manually as r27865 (presubmit successful).