Issue 21381002: Implement mirror equality on the VM. Resolves issue 12026.

Issue 21381002: Implement mirror equality on the VM. Resolves issue 12026. (Closed)

Created:
7 years, 4 months ago by rmacnak

Modified:
7 years, 4 months ago

Reviewers:
ahe, siva

CC:
reviews_dartlang.org, ahe, Michael Lippautz (Google)

Base URL:
http://dart.googlecode.com/svn/branches/bleeding_edge/dart/

Visibility:
Public.

More Reviews

Description

Implement mirror equality on the VM. Resolves issue 12026. R=asiva@google.com Committed: https://code.google.com/p/dart/source/detail?r=25711

Patch Set 1 : #

Total comments: 7

Created: 7 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+39 lines, -2 lines)			Patch
M	runtime/lib/mirror_reference.dart	View	1 chunk	+2 lines, -0 lines	0 comments	Download
M	runtime/lib/mirrors.cc	View	1 chunk	+7 lines, -0 lines	2 comments	Download
M	runtime/lib/mirrors_impl.dart	View	5 chunks	+29 lines, -1 line	5 comments	Download
M	runtime/vm/bootstrap_natives.h	View	1 chunk	+1 line, -0 lines	0 comments	Download
M	tests/lib/lib.status	View	1 chunk	+0 lines, -1 line	0 comments	Download

Messages

Total messages: 9 (0 generated)

Expand Messages | Collapse Messages

rmacnak

https://codereview.chromium.org/21381002/diff/5001/runtime/lib/mirrors.cc File runtime/lib/mirrors.cc (right): https://codereview.chromium.org/21381002/diff/5001/runtime/lib/mirrors.cc#newcode460 runtime/lib/mirrors.cc:460: return Bool::Get(a.referent() == b.referent()); Assert no GC? Identical_comparison doesn't. ...

7 years, 4 months ago (2013-07-31 18:38:00 UTC) #1

siva

7 years, 4 months ago (2013-08-01 00:05:06 UTC) #2

rmacnak

On 2013/08/01 00:05:06, siva wrote: > https://codereview.chromium.org/21381002/diff/5001/runtime/lib/mirrors.cc > File runtime/lib/mirrors.cc (right): > > https://codereview.chromium.org/21381002/diff/5001/runtime/lib/mirrors.cc#newcode460 > ...

7 years, 4 months ago (2013-08-01 00:16:23 UTC) #4

On 2013/08/01 00:05:06, siva wrote:
> https://codereview.chromium.org/21381002/diff/5001/runtime/lib/mirrors.cc
> File runtime/lib/mirrors.cc (right):
> 
>
https://codereview.chromium.org/21381002/diff/5001/runtime/lib/mirrors.cc#new...
> runtime/lib/mirrors.cc:460: return Bool::Get(a.referent() == b.referent());
> Sure you can add a NoGCScope here if you want.
> 
> On 2013/07/31 18:38:01, Ryan Macnak wrote:
> > Assert no GC? Identical_comparison doesn't.
> 
>
https://codereview.chromium.org/21381002/diff/5001/runtime/lib/mirrors_impl.dart
> File runtime/lib/mirrors_impl.dart (right):
> 
>
https://codereview.chromium.org/21381002/diff/5001/runtime/lib/mirrors_impl.d...
> runtime/lib/mirrors_impl.dart:304: int get hashCode => _reflectee.hashCode;
> Could you explain under what circumstances reflectee.hashCode can raise an
> exception?
> 
> On 2013/07/31 18:38:01, Ryan Macnak wrote:
> > _reflectee.hashCode could raise an exception. It this worth being defensive
> > about?
> 
>
https://codereview.chromium.org/21381002/diff/5001/runtime/lib/mirrors_impl.d...
> runtime/lib/mirrors_impl.dart:608: return this.runtimeType ==
other.runtimeType
> &&
> Ditto question about when an exception is raised.
> 
> On 2013/07/31 18:38:01, Ryan Macnak wrote:
> > other.runtimeType should raise an exception. Is this worth being defensive
> > about?

The reflectee of an InstanceMirror is an arbitrary object, which may have
overridden hashCode. The argument to equals is an arbitrary object, which may
have overridden runtimeType. Unhandled exceptions face a similar problem with
toString() and use a safeToString().  A safe runtimeType is easy to add (and in
fact now realize we already have it in order to implement InstanceMirror.type),
but a safe identityHash is more complicated because Dart objects don't have a
real identityHash (i.e., in their header) so we'd have to coordinate with the
private Expando in dart:core that keeps these hashes. Or have another Expando in
dart:mirrors.

siva

On 2013/08/01 00:16:23, Ryan Macnak wrote: > On 2013/08/01 00:05:06, siva wrote: > > https://codereview.chromium.org/21381002/diff/5001/runtime/lib/mirrors.cc ...

7 years, 4 months ago (2013-08-01 01:05:24 UTC) #5

On 2013/08/01 00:16:23, Ryan Macnak wrote:
> On 2013/08/01 00:05:06, siva wrote:
> > https://codereview.chromium.org/21381002/diff/5001/runtime/lib/mirrors.cc
> > File runtime/lib/mirrors.cc (right):
> > 
> >
>
https://codereview.chromium.org/21381002/diff/5001/runtime/lib/mirrors.cc#new...
> > runtime/lib/mirrors.cc:460: return Bool::Get(a.referent() == b.referent());
> > Sure you can add a NoGCScope here if you want.
> > 
> > On 2013/07/31 18:38:01, Ryan Macnak wrote:
> > > Assert no GC? Identical_comparison doesn't.
> > 
> >
>
https://codereview.chromium.org/21381002/diff/5001/runtime/lib/mirrors_impl.dart
> > File runtime/lib/mirrors_impl.dart (right):
> > 
> >
>
https://codereview.chromium.org/21381002/diff/5001/runtime/lib/mirrors_impl.d...
> > runtime/lib/mirrors_impl.dart:304: int get hashCode => _reflectee.hashCode;
> > Could you explain under what circumstances reflectee.hashCode can raise an
> > exception?
> > 
> > On 2013/07/31 18:38:01, Ryan Macnak wrote:
> > > _reflectee.hashCode could raise an exception. It this worth being
defensive
> > > about?
> > 
> >
>
https://codereview.chromium.org/21381002/diff/5001/runtime/lib/mirrors_impl.d...
> > runtime/lib/mirrors_impl.dart:608: return this.runtimeType ==
> other.runtimeType
> > &&
> > Ditto question about when an exception is raised.
> > 
> > On 2013/07/31 18:38:01, Ryan Macnak wrote:
> > > other.runtimeType should raise an exception. Is this worth being defensive
> > > about?
> 
> The reflectee of an InstanceMirror is an arbitrary object, which may have
> overridden hashCode. The argument to equals is an arbitrary object, which may
> have overridden runtimeType. Unhandled exceptions face a similar problem with
> toString() and use a safeToString().  A safe runtimeType is easy to add (and
in
> fact now realize we already have it in order to implement
InstanceMirror.type),
> but a safe identityHash is more complicated because Dart objects don't have a
> real identityHash (i.e., in their header) so we'd have to coordinate with the
> private Expando in dart:core that keeps these hashes. Or have another Expando
in
> dart:mirrors.

Okay this is just a case of user code overriding the methods and throwing
exceptions.
I think that should be fine, I don't think it is necessary to protect against
that.

rmacnak

Committed patchset #1 manually as r25711 (presubmit successful).

7 years, 4 months ago (2013-08-01 16:40:15 UTC) #7

ahe

7 years, 4 months ago (2013-08-06 15:28:11 UTC) #9

Message was sent while issue was closed.

https://codereview.chromium.org/21381002/diff/5001/runtime/lib/mirrors_impl.dart
File runtime/lib/mirrors_impl.dart (right):

https://codereview.chromium.org/21381002/diff/5001/runtime/lib/mirrors_impl.d...
runtime/lib/mirrors_impl.dart:304: int get hashCode => _reflectee.hashCode;
This hashCode implementation is not compatible with operator ==.
You really should use hashCode from Object.

Expand Messages | Collapse Messages