Implement InstanceMirror.== in dart2js and InstanceMirror.hashCode in the VM and …

Implement InstanceMirror.== in dart2js and InstanceMirror.hashCode in the VM and dart2js.

BUG=http://dartbug.com/12909
BUG=http://dartbug.com/12919
R=ahe@google.com, asiva@google.com

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

[rmacnak, 2013-08-30 20:23:58]

Waiting on https://chromiumcodereview.appspot.com/23657002/.
Will split test to cover VM and dart2js.

[ahe, 2013-08-30 20:40:14]

Test and dart2js, LGTM!

https://chromiumcodereview.appspot.com/23460013/diff/3001/sdk/lib/_internal/l...
File sdk/lib/_internal/lib/js_mirrors.dart (right):

https://chromiumcodereview.appspot.com/23460013/diff/3001/sdk/lib/_internal/l...
sdk/lib/_internal/lib/js_mirrors.dart:33: getInterceptor;
I think you can remove this import if my suggestion below works.

https://chromiumcodereview.appspot.com/23460013/diff/3001/sdk/lib/_internal/l...
sdk/lib/_internal/lib/js_mirrors.dart:692: return getInterceptor(reflectee) is
Interceptor
I think other native types, such as DivElement, will return true here.

So I think it would be better to use this test:

return (JS('bool', 'typeof # == "object"', reflectee)) ?
Primitives.objectHashCode(reflectee) : reflectee.hashCode;

https://chromiumcodereview.appspot.com/23460013/diff/3001/tests/lib/mirrors/e...
File tests/lib/mirrors/equality_test.dart (right):

https://chromiumcodereview.appspot.com/23460013/diff/3001/tests/lib/mirrors/e...
tests/lib/mirrors/equality_test.dart:60: checkEquality([
Could you avoid passing in a list, and instead call checkEquality multiple
times.

I know it is a bit more verbose and tedious, but it really helps when debugging
dart2js output that the stack trace will point you to test that is failing.

[rmacnak, 2013-08-30 21:08:29]

https://chromiumcodereview.appspot.com/23460013/diff/3001/sdk/lib/_internal/l...
File sdk/lib/_internal/lib/js_mirrors.dart (right):

https://chromiumcodereview.appspot.com/23460013/diff/3001/sdk/lib/_internal/l...
sdk/lib/_internal/lib/js_mirrors.dart:33: getInterceptor;
On 2013/08/30 20:40:14, ahe wrote:
> I think you can remove this import if my suggestion below works.

Done.

https://chromiumcodereview.appspot.com/23460013/diff/3001/sdk/lib/_internal/l...
sdk/lib/_internal/lib/js_mirrors.dart:692: return getInterceptor(reflectee) is
Interceptor
On 2013/08/30 20:40:14, ahe wrote:
> I think other native types, such as DivElement, will return true here.
> 
> So I think it would be better to use this test:
> 
> return (JS('bool', 'typeof # == "object"', reflectee)) ?
> Primitives.objectHashCode(reflectee) : reflectee.hashCode;

Done, but I have to also separate out null. typeof null == "object", but
Primitives.objectHashCode(null) does not work (TypeError: Cannot read property
'$identityHash' of null).

https://chromiumcodereview.appspot.com/23460013/diff/3001/tests/lib/mirrors/e...
File tests/lib/mirrors/equality_test.dart (right):

https://chromiumcodereview.appspot.com/23460013/diff/3001/tests/lib/mirrors/e...
tests/lib/mirrors/equality_test.dart:60: checkEquality([
On 2013/08/30 20:40:14, ahe wrote:
> Could you avoid passing in a list, and instead call checkEquality multiple
> times.
> 
> I know it is a bit more verbose and tedious, but it really helps when
debugging
> dart2js output that the stack trace will point you to test that is failing.

The number of cases is quadratic. Notice I am passing in unique strings to
identify each of these expressions, so that when the test fails you can easily
identify which pair of mirrors is problematic.

[ahe, 2013-08-30 21:11:13]

https://chromiumcodereview.appspot.com/23460013/diff/3001/tests/lib/mirrors/e...
File tests/lib/mirrors/equality_test.dart (right):

https://chromiumcodereview.appspot.com/23460013/diff/3001/tests/lib/mirrors/e...
tests/lib/mirrors/equality_test.dart:60: checkEquality([
On 2013/08/30 21:08:29, Ryan Macnak wrote:
> On 2013/08/30 20:40:14, ahe wrote:
> > Could you avoid passing in a list, and instead call checkEquality multiple
> > times.
> > 
> > I know it is a bit more verbose and tedious, but it really helps when
> debugging
> > dart2js output that the stack trace will point you to test that is failing.
> 
> The number of cases is quadratic. Notice I am passing in unique strings to
> identify each of these expressions, so that when the test fails you can easily
> identify which pair of mirrors is problematic.

OK

[rmacnak, 2013-08-30 22:32:06]

Merged with committed parent CL and split test to keep dart2js covered.

[ahe, 2013-09-03 13:05:58]

Everything but the C++ code, LGTM!

https://chromiumcodereview.appspot.com/23460013/diff/13001/sdk/lib/_internal/...
File sdk/lib/_internal/lib/js_mirrors.dart (right):

https://chromiumcodereview.appspot.com/23460013/diff/13001/sdk/lib/_internal/...
sdk/lib/_internal/lib/js_mirrors.dart:694: :
Primitives.objectHashCode(reflectee);
I think you need to change the hashCode by xoring with some other number.

Otherwise, the hashCode will be the same as for the reflectee in many cases.

[rmacnak, 2013-09-03 17:59:47]

On 2013/09/03 13:05:58, ahe wrote:
> Everything but the C++ code, LGTM!
> 
>
https://chromiumcodereview.appspot.com/23460013/diff/13001/sdk/lib/_internal/...
> File sdk/lib/_internal/lib/js_mirrors.dart (right):
> 
>
https://chromiumcodereview.appspot.com/23460013/diff/13001/sdk/lib/_internal/...
> sdk/lib/_internal/lib/js_mirrors.dart:694: :
> Primitives.objectHashCode(reflectee);
> I think you need to change the hashCode by xoring with some other number.
> 
> Otherwise, the hashCode will be the same as for the reflectee in many cases.

Done.

[siva, 2013-09-03 19:43:42]

lgtm

https://chromiumcodereview.appspot.com/23460013/diff/19001/runtime/lib/mirror...
File runtime/lib/mirrors.cc (right):

https://chromiumcodereview.appspot.com/23460013/diff/19001/runtime/lib/mirror...
runtime/lib/mirrors.cc:756: ObjectStore* object_store =
Isolate::Current()->object_store();
You should be able to write this as
ObjectStore* object_store = isolate->object_store();

(the current isolate is a parameter to native functions)

https://chromiumcodereview.appspot.com/23460013/diff/19001/runtime/lib/mirror...
runtime/lib/mirrors.cc:759:
Function::Handle(cls.LookupDynamicFunction(Symbols::hashCode()));
Also pass in the isolate to these handles as it is available:
Class::Handle(isolate, ...);
Function::Handle(isolate, ...)
Array::Handle(isolate, ...)

https://chromiumcodereview.appspot.com/23460013/diff/19001/runtime/lib/mirror...
File runtime/lib/mirrors_impl.dart (right):

https://chromiumcodereview.appspot.com/23460013/diff/19001/runtime/lib/mirror...
runtime/lib/mirrors_impl.dart:297: }
Maybe for later, it might be worth caching the hash code to avoid repeated
native calls.

[rmacnak, 2013-09-03 20:10:54]

https://chromiumcodereview.appspot.com/23460013/diff/19001/runtime/lib/mirror...
File runtime/lib/mirrors.cc (right):

https://chromiumcodereview.appspot.com/23460013/diff/19001/runtime/lib/mirror...
runtime/lib/mirrors.cc:756: ObjectStore* object_store =
Isolate::Current()->object_store();
On 2013/09/03 19:43:43, siva wrote:
> You should be able to write this as
> ObjectStore* object_store = isolate->object_store();
> 
> (the current isolate is a parameter to native functions)

Done.

https://chromiumcodereview.appspot.com/23460013/diff/19001/runtime/lib/mirror...
runtime/lib/mirrors.cc:759:
Function::Handle(cls.LookupDynamicFunction(Symbols::hashCode()));
On 2013/09/03 19:43:43, siva wrote:
> Also pass in the isolate to these handles as it is available:
> Class::Handle(isolate, ...);
> Function::Handle(isolate, ...)
> Array::Handle(isolate, ...)

Done.

[rmacnak, 2013-09-03 20:11:18]

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