Issue 11345031: [dart2dart] Support cosntructor redirects.

Issue 11345031: [dart2dart] Support cosntructor redirects. (Closed)

Created:
8 years, 1 month ago by Roman

Modified:
8 years, 1 month ago

Reviewers:
Anton Muhin

CC:
reviews_dartlang.org

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

Visibility:
Public.

More Reviews

Description

[dart2dart] Support cosntructor redirects. Committed: https://code.google.com/p/dart/source/detail?r=14297

Patch Set 2 : fix dart_backend_test #

Total comments: 10

Patch Set 3 : #

Created: 8 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+28 lines, -168 lines)			Patch
M	lib/compiler/implementation/dart_backend/placeholder_collector.dart	View	1 2	5 chunks	+20 lines, -23 lines	0 comments	Download
M	lib/compiler/implementation/dart_backend/renamer.dart	View	1 2	4 chunks	+8 lines, -5 lines	0 comments	Download
M	tests/co19/co19-dart2dart.status	View		1 chunk	+0 lines, -16 lines	0 comments	Download
M	tests/compiler/dart2js/dart_backend_test.dart	View	1	6 chunks	+0 lines, -112 lines	0 comments	Download
M	tests/language/language.status	View		1 chunk	+0 lines, -12 lines	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

Anton Muhin

https://codereview.chromium.org/11345031/diff/6/lib/compiler/implementation/dart_backend/placeholder_collector.dart File lib/compiler/implementation/dart_backend/placeholder_collector.dart (right): https://codereview.chromium.org/11345031/diff/6/lib/compiler/implementation/dart_backend/placeholder_collector.dart#newcode31 lib/compiler/implementation/dart_backend/placeholder_collector.dart:31: ConstructorPlaceholder(this.node, this.type, this.isRedirectingCall) { it looks like as of ...

8 years, 1 month ago (2012-10-30 15:23:44 UTC) #2

Roman

8 years, 1 month ago (2012-10-30 15:52:09 UTC) #3

https://codereview.chromium.org/11345031/diff/6/lib/compiler/implementation/d...
File lib/compiler/implementation/dart_backend/placeholder_collector.dart
(right):

https://codereview.chromium.org/11345031/diff/6/lib/compiler/implementation/d...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:31:
ConstructorPlaceholder(this.node, this.type, this.isRedirectingCall) {
On 2012/10/30 15:23:44, Anton Muhin wrote:
> it looks like as of now in all the call sites you pass a constant as
> isRedirectingCall.
> 
> I would suggest to add named constructor ConstructorPlace.redirecting and
> corresponding helper method, that would make code easier to read imho

Done.

https://codereview.chromium.org/11345031/diff/6/lib/compiler/implementation/d...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:55:
collector.makeConstructorPlaceholder(node.selector, element, null, true);
On 2012/10/30 15:23:44, Anton Muhin wrote:
> my original understanding of constructor placeholders was a collection of
> constructor declarations to rename.
> 
> Now it includes invocations too.
> 
> Should they go into the same place?
> 
> Cannot we add redirecting constructor invocations into member renames where
they
> apparently should belong?

1) We cannot add declarations to one placeholder type and invocations to
another, that way it would be quite difficult to rename them consistently.
2) Originally both constructor invocations and declarations made their way into
constructor placeholders, invocations - from visitNewExpression. This CL makes
constructor placeholders for redirecting invocations.

https://codereview.chromium.org/11345031/diff/6/lib/compiler/implementation/d...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:172: // Two
complicated cases for class/interface renaming:
On 2012/10/30 15:23:44, Anton Muhin wrote:
> cannot this comment go away now?

Done.

https://codereview.chromium.org/11345031/diff/6/lib/compiler/implementation/d...
File lib/compiler/implementation/dart_backend/renamer.dart (right):

https://codereview.chromium.org/11345031/diff/6/lib/compiler/implementation/d...
lib/compiler/implementation/dart_backend/renamer.dart:112: if
(!placeholder.isRedirectingCall) {
On 2012/10/30 15:23:44, Anton Muhin wrote:
> where do we add super./this. prefix?

Nowhere, the placeholder is placed for selector node:
receiver: "this" / "super"
selector: constructor name

https://codereview.chromium.org/11345031/diff/6/lib/compiler/implementation/d...
lib/compiler/implementation/dart_backend/renamer.dart:126: if
(placeholder.isRedirectingCall) throw 'e';
On 2012/10/30 15:23:44, Anton Muhin wrote:
> isn't assert enough?

Sorry, that was my way to ensure that placeholder is not redirecting call during
test run :) Removed.

lgtm

Expand Messages | Collapse Messages