Issue 14098009: Fix x.runtimeType for native classes

Issue 14098009: Fix x.runtimeType for native classes (Closed)

Created:
7 years, 8 months ago by sra1

Modified:
7 years, 8 months ago

Reviewers:
karlklose, ngeoffray

CC:
reviews_dartlang.org

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

Visibility:
Public.

More Reviews

Description

Fix x.runtimeType for native classes 1. Make sure native classes inherit from Interceptor so they pick up get runtimeTime from there. 2. Move some duplicated code into js_rti. Committed: https://code.google.com/p/dart/source/detail?r=21963

Patch Set 1 : #

Patch Set 2 : #

Total comments: 10

Patch Set 3 : #

Created: 7 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+98 lines, -25 lines)			Patch
M	sdk/lib/_internal/compiler/implementation/compiler.dart	View	1	1 chunk	+2 lines, -0 lines	0 comments	Download
M	sdk/lib/_internal/compiler/implementation/js_backend/backend.dart	View	1	2 chunks	+6 lines, -0 lines	0 comments	Download
M	sdk/lib/_internal/compiler/implementation/lib/core_patch.dart	View		2 chunks	+2 lines, -6 lines	0 comments	Download
M	sdk/lib/_internal/compiler/implementation/lib/interceptors.dart	View		2 chunks	+2 lines, -3 lines	0 comments	Download
M	sdk/lib/_internal/compiler/implementation/lib/js_array.dart	View		1 chunk	+0 lines, -5 lines	0 comments	Download
M	sdk/lib/_internal/compiler/implementation/lib/js_rti.dart	View		2 chunks	+6 lines, -1 line	0 comments	Download
M	sdk/lib/_internal/compiler/implementation/resolution/members.dart	View	1 2	1 chunk	+15 lines, -10 lines	0 comments	Download
A	tests/compiler/dart2js_native/runtimetype_test.dart	View	1 2	1 chunk	+65 lines, -0 lines	0 comments	Download

Messages

Total messages: 6 (0 generated)

Expand Messages | Collapse Messages

ngeoffray

DBC https://chromiumcodereview.appspot.com/14098009/diff/5001/sdk/lib/_internal/compiler/implementation/resolution/members.dart File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right): https://chromiumcodereview.appspot.com/14098009/diff/5001/sdk/lib/_internal/compiler/implementation/resolution/members.dart#newcode3192 sdk/lib/_internal/compiler/implementation/resolution/members.dart:3192: if (element.supertype == null && element != compiler.objectClass) ...

7 years, 8 months ago (2013-04-24 06:58:13 UTC) #3

karlklose

LGTM if you address Nicolas' comments. https://chromiumcodereview.appspot.com/14098009/diff/5001/sdk/lib/_internal/compiler/implementation/resolution/members.dart File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right): https://chromiumcodereview.appspot.com/14098009/diff/5001/sdk/lib/_internal/compiler/implementation/resolution/members.dart#newcode3191 sdk/lib/_internal/compiler/implementation/resolution/members.dart:3191: // If the ...

7 years, 8 months ago (2013-04-24 14:27:28 UTC) #4

sra1

Committed patchset #3 manually as r21963 (presubmit successful).

7 years, 8 months ago (2013-04-24 17:32:23 UTC) #5

sra1

7 years, 8 months ago (2013-04-24 17:32:43 UTC) #6

Message was sent while issue was closed.

https://chromiumcodereview.appspot.com/14098009/diff/5001/sdk/lib/_internal/c...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://chromiumcodereview.appspot.com/14098009/diff/5001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3191: // If
the super type isn't specified, we make it Object.
On 2013/04/24 14:27:28, karlklose wrote:
> Please update the comment.

Done.

https://chromiumcodereview.appspot.com/14098009/diff/5001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3192: if
(element.supertype == null && element != compiler.objectClass) {
On 2013/04/24 06:58:13, ngeoffray wrote:
> This code looks kind of convoluted. I'd prefer having a isNative check
> explicitly.

Since this is the resolver we should use the backend otherwise we would break
dart2dart.  So 'isNative()' should not really be here.

https://chromiumcodereview.appspot.com/14098009/diff/5001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3193: final
superElement = compiler.backend.defaultSuperclass(element);
On 2013/04/24 06:58:13, ngeoffray wrote:
> final -> ClassElement

Done.

https://chromiumcodereview.appspot.com/14098009/diff/5001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3194: if
(element != superElement) {  // I.e. don't make Object extend Object.
On 2013/04/24 06:58:13, ngeoffray wrote:
> You already made that test (don't make Object extend Object) line 3192.

Not quite true.  I'll clarify the comment. We dont want Interceptor to extend
Interceptor either.

https://chromiumcodereview.appspot.com/14098009/diff/5001/tests/compiler/dart...
File tests/compiler/dart2js_native/runtimetype_test.dart (right):

https://chromiumcodereview.appspot.com/14098009/diff/5001/tests/compiler/dart...
tests/compiler/dart2js_native/runtimetype_test.dart:1: // Copyright (c) 2011,
the Dart project authors.  Please see the AUTHORS file
On 2013/04/24 14:27:28, karlklose wrote:
> 2011 -> 2013.

Done.

Expand Messages | Collapse Messages