A visitor that builds a map from node to "Usage".

A visitor that builds a map from node to "Usage".
A renamer just renames "usages" according to its type.
This allows us to split various send-guessing logic and renaming.
No scopes yet.

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

[Anton Muhin, 2012-08-07 12:32:10]

Overall looks neat, 1st round of comments

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
File lib/compiler/implementation/dart_backend/backend.dart (right):

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/backend.dart:76: || element is
SynthesizedConstructorElement
why this is not a part of shouldOutput?

and we have pretty much the same logic in Emitter.outputElement, shouldn't we
unify it?

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
File lib/compiler/implementation/dart_backend/renamer.dart (right):

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/renamer.dart:21:
renameElement(compiler.mainApp.find(Compiler.MAIN));
will it always work?  there is this --allow-mock-compilation thing

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/renamer.dart:34:
"${element.getEnclosingClass().type.name.slowToString()}\$";
nit: single quotes, please

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/renamer.dart:35: if
(element.isGenerativeConstructor()) {
why you have this difference?   Why you process A.foo ctor differently from
A.foo factory?

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/renamer.dart:40: // TODO(smok): Remove
this fragile check, $ can appear in ID.
cannot you use cachedNode to understand the name of the constructor?

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
File lib/compiler/implementation/dart_backend/usager.dart (right):

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/usager.dart:18: if (element !== null &&
element.isTopLevel()) {
maybe use early return: if (element === null || element.isTopLevel()) {
  node.visitChildren(usager);
  return;
}

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/usager.dart:20: // Library reference
prefix. Omit it.
please, add an assert, that receiver resolves to prefix:
assert(elements[node.receover].isPrefix())

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/usager.dart:24:
usager.visit(node.argumentsNode);
I am not quite happy we duplicate visitChildren here: if somebody updates it,
we're in trouble

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/usager.dart:30: R visitStaticSend(Send
node) {
isn't that too complicated?

e.g. shouldn't we rename elements being output immediately, w/o resorting to the
things below?

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/usager.dart:114: if (result == null) {
nit: === null

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/usager.dart:145: R visit(Node node) =>
(node == null) ? null : node.accept(this);
nit: === null

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/usager.dart:147: R visitBlock(Block
node) {
arrow syntax, your variant returns null.

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/usager.dart:147: R visitBlock(Block
node) {
cannot you reuse AbstractVisitor?

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
File lib/compiler/implementation/tree/unparser.dart (right):

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
lib/compiler/implementation/tree/unparser.dart:317: visit(node.typeName);
visitChildren?

https://chromiumcodereview.appspot.com/10836128/diff/3001/tests/language/lang...
File tests/language/language.status (right):

https://chromiumcodereview.appspot.com/10836128/diff/3001/tests/language/lang...
tests/language/language.status:145: double_int_addition_test: Fail
why?

[Roman, 2012-08-07 15:44:03]

Changed significant visitor parts to your approach.

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
File lib/compiler/implementation/dart_backend/backend.dart (right):

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/backend.dart:76: || element is
SynthesizedConstructorElement
On 2012/08/07 12:32:11, Anton Mukhin wrote:
> why this is not a part of shouldOutput?
> 
> and we have pretty much the same logic in Emitter.outputElement, shouldn't we
> unify it?

We should, but I'm not sure what would be the nice way to do that. We don't have
this check in shouldOutput, because if we see AbstractFieldMember or
constructor, we still should call addMemberToClass(), otherwise class A that has
only constructor used, will not be emitted.

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
File lib/compiler/implementation/dart_backend/renamer.dart (right):

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/renamer.dart:21:
renameElement(compiler.mainApp.find(Compiler.MAIN));
On 2012/08/07 12:32:11, Anton Mukhin wrote:
> will it always work?  there is this --allow-mock-compilation thing

Compiler does the same thing. Main is required anyway, isn't it?

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/renamer.dart:35: if
(element.isGenerativeConstructor()) {
On 2012/08/07 12:32:11, Anton Mukhin wrote:
> why you have this difference?   Why you process A.foo ctor differently from
> A.foo factory?

I think this code is broken. We don't plan to rename constructor names soon
anyway, so I just removed "constructor name usage", this made naming_test pass.

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
File lib/compiler/implementation/dart_backend/usager.dart (right):

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/usager.dart:18: if (element !== null &&
element.isTopLevel()) {
On 2012/08/07 12:32:11, Anton Mukhin wrote:
> maybe use early return: if (element === null || element.isTopLevel()) {
>   node.visitChildren(usager);
>   return;
> }

Done.

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/usager.dart:20: // Library reference
prefix. Omit it.
On 2012/08/07 12:32:11, Anton Mukhin wrote:
> please, add an assert, that receiver resolves to prefix:
> assert(elements[node.receover].isPrefix())

Done.

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/usager.dart:24:
usager.visit(node.argumentsNode);
On 2012/08/07 12:32:11, Anton Mukhin wrote:
> I am not quite happy we duplicate visitChildren here: if somebody updates it,
> we're in trouble

Changed to visitChildren, it should not matter much here.

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/usager.dart:30: R visitStaticSend(Send
node) {
On 2012/08/07 12:32:11, Anton Mukhin wrote:
> isn't that too complicated?
> 
> e.g. shouldn't we rename elements being output immediately, w/o resorting to
the
> things below?

I will use magic from your CL to make it better.

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/usager.dart:114: if (result == null) {
On 2012/08/07 12:32:11, Anton Mukhin wrote:
> nit: === null

Done.

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/usager.dart:145: R visit(Node node) =>
(node == null) ? null : node.accept(this);
On 2012/08/07 12:32:11, Anton Mukhin wrote:
> nit: === null

Done.

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
File lib/compiler/implementation/tree/unparser.dart (right):

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
lib/compiler/implementation/tree/unparser.dart:317: visit(node.typeName);
On 2012/08/07 12:32:11, Anton Mukhin wrote:
> visitChildren?

The problem here is the nature of visitChildren(its impl) {
  typeName.accept(visitor);
  typeArguments.accept(visitor);
}

accept dispatches to visitor.visitSend or visitIdentifier(), but we need to go
through visit() to rename stuff.

[Anton Muhin, 2012-08-07 16:00:30]

Interrupted review, I'll finish later.  But you may want to address some of my
comments now :)

https://chromiumcodereview.appspot.com/10836128/diff/8001/lib/compiler/implem...
File lib/compiler/implementation/dart_backend/renamer.dart (right):

https://chromiumcodereview.appspot.com/10836128/diff/8001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/renamer.dart:9: class
ConflictingRenamer extends Renamer {
conflicting renamer now looks like a bad name.  Overall, given that we have now
two obviously separate parts: CR.rename and CR.renameElement, it might be time
to have a clear split in this regard, possibly merging logic of CR.rename
directly into visitor.

https://chromiumcodereview.appspot.com/10836128/diff/8001/lib/compiler/implem...
File lib/compiler/implementation/dart_backend/usage.dart (right):

https://chromiumcodereview.appspot.com/10836128/diff/8001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/usage.dart:5: class UsageKind {
where do you use kinds?

https://chromiumcodereview.appspot.com/10836128/diff/8001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/usage.dart:31:
renamer.renameElement(type.element);
do you need to pass renamer if you may pass a function String renamer(Element
element)?

https://chromiumcodereview.appspot.com/10836128/diff/8001/lib/compiler/implem...
File lib/compiler/implementation/dart_backend/usager.dart (right):

https://chromiumcodereview.appspot.com/10836128/diff/8001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/usager.dart:10: R visitSuperSend(Send
node) => node.visitChildren(usager);
why do you need to visit children?

https://chromiumcodereview.appspot.com/10836128/diff/8001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/usager.dart:10: R visitSuperSend(Send
node) => node.visitChildren(usager);
R should be now String

https://chromiumcodereview.appspot.com/10836128/diff/8001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/usager.dart:21: return;
you promise to return String, but explicitly return null, which is bad.  Worse,
below you use implicit returns which return nulls.

https://chromiumcodereview.appspot.com/10836128/diff/8001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/usager.dart:65: bool
isDartCoreLib(Compiler compiler, LibraryElement libraryElement) {
I don't think it should belong to any class.

https://chromiumcodereview.appspot.com/10836128/diff/8001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/usager.dart:108: var result =
treeElements.getType(typeAnnotation);
please, TODO to have better type resolution.

https://chromiumcodereview.appspot.com/10836128/diff/8001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/usager.dart:115: void useType(Node
node, Type type) {
as we discussed, use is probably wrong name

[Anton Muhin, 2012-08-07 16:24:08]

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
File lib/compiler/implementation/tree/unparser.dart (right):

https://chromiumcodereview.appspot.com/10836128/diff/3001/lib/compiler/implem...
lib/compiler/implementation/tree/unparser.dart:317: visit(node.typeName);
TODO here and a fix for visitChildren in a separate CL?

On 2012/08/07 15:44:03, Roman wrote:
> On 2012/08/07 12:32:11, Anton Mukhin wrote:
> > visitChildren?
> 
> The problem here is the nature of visitChildren(its impl) {
>   typeName.accept(visitor);
>   typeArguments.accept(visitor);
> }
> 
> accept dispatches to visitor.visitSend or visitIdentifier(), but we need to go
> through visit() to rename stuff.

https://chromiumcodereview.appspot.com/10836128/diff/8001/lib/compiler/implem...
File lib/compiler/implementation/dart_backend/usager.dart (right):

https://chromiumcodereview.appspot.com/10836128/diff/8001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/usager.dart:91: if
(nameNode.token.slowToString() == enclosingClass.name.slowToString()) {
please, add a comment that this if is needed for cases like class C implements I
{ I(); } (I know it's missing in my cls)

https://chromiumcodereview.appspot.com/10836128/diff/8001/lib/compiler/implem...
File lib/compiler/implementation/tree/renamer.dart (right):

https://chromiumcodereview.appspot.com/10836128/diff/8001/lib/compiler/implem...
lib/compiler/implementation/tree/renamer.dart:9: class Renamer {
do we need this interface?  won't typedef be enough?

[Roman, 2012-08-08 06:46:14]

https://chromiumcodereview.appspot.com/10836128/diff/8001/lib/compiler/implem...
File lib/compiler/implementation/dart_backend/renamer.dart (right):

https://chromiumcodereview.appspot.com/10836128/diff/8001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/renamer.dart:9: class
ConflictingRenamer extends Renamer {
On 2012/08/07 16:00:30, Anton Mukhin wrote:
> conflicting renamer now looks like a bad name.  Overall, given that we have
now
> two obviously separate parts: CR.rename and CR.renameElement, it might be time
> to have a clear split in this regard, possibly merging logic of CR.rename
> directly into visitor.

I'm not sure. I think it is even better name that was before, earlier it was
deciding what to rename, now it only renames according to placeholder info.

https://chromiumcodereview.appspot.com/10836128/diff/8001/lib/compiler/implem...
File lib/compiler/implementation/dart_backend/usage.dart (right):

https://chromiumcodereview.appspot.com/10836128/diff/8001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/usage.dart:5: class UsageKind {
On 2012/08/07 16:00:30, Anton Mukhin wrote:
> where do you use kinds?

Removed

https://chromiumcodereview.appspot.com/10836128/diff/8001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/usage.dart:31:
renamer.renameElement(type.element);
On 2012/08/07 16:00:30, Anton Mukhin wrote:
> do you need to pass renamer if you may pass a function String renamer(Element
> element)?

I would like to leave more info to Placeholder about renamer. Currently we use
only renameElement, but later I think it will be more sophisticated.

https://chromiumcodereview.appspot.com/10836128/diff/8001/lib/compiler/implem...
File lib/compiler/implementation/dart_backend/usager.dart (right):

https://chromiumcodereview.appspot.com/10836128/diff/8001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/usager.dart:10: R visitSuperSend(Send
node) => node.visitChildren(usager);
On 2012/08/07 16:00:30, Anton Mukhin wrote:
> why do you need to visit children?

Fixed, visiting children now in PlaceholderCollector.

https://chromiumcodereview.appspot.com/10836128/diff/8001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/usager.dart:10: R visitSuperSend(Send
node) => node.visitChildren(usager);
On 2012/08/07 16:00:30, Anton Mukhin wrote:
> R should be now String

Actually it should not return anything.

https://chromiumcodereview.appspot.com/10836128/diff/8001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/usager.dart:65: bool
isDartCoreLib(Compiler compiler, LibraryElement libraryElement) {
On 2012/08/07 16:00:30, Anton Mukhin wrote:
> I don't think it should belong to any class.

Forgot to remove.

https://chromiumcodereview.appspot.com/10836128/diff/8001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/usager.dart:91: if
(nameNode.token.slowToString() == enclosingClass.name.slowToString()) {
On 2012/08/07 16:24:08, Anton Mukhin wrote:
> please, add a comment that this if is needed for cases like class C implements
I
> { I(); } (I know it's missing in my cls)

Done.

https://chromiumcodereview.appspot.com/10836128/diff/8001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/usager.dart:108: var result =
treeElements.getType(typeAnnotation);
On 2012/08/07 16:00:30, Anton Mukhin wrote:
> please, TODO to have better type resolution.

Done.

https://chromiumcodereview.appspot.com/10836128/diff/8001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/usager.dart:115: void useType(Node
node, Type type) {
On 2012/08/07 16:00:30, Anton Mukhin wrote:
> as we discussed, use is probably wrong name

renamed to placeholderForType

https://chromiumcodereview.appspot.com/10836128/diff/8001/lib/compiler/implem...
File lib/compiler/implementation/tree/renamer.dart (right):

https://chromiumcodereview.appspot.com/10836128/diff/8001/lib/compiler/implem...
lib/compiler/implementation/tree/renamer.dart:9: class Renamer {
On 2012/08/07 16:24:08, Anton Mukhin wrote:
> do we need this interface?  won't typedef be enough?

Removed.

[Anton Muhin, 2012-08-08 08:27:19]

getting really close

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
File lib/compiler/implementation/dart_backend/backend.dart (right):

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/dart_backend/backend.dart:76: || element is
SynthesizedConstructorElement
can we move this logic into collector than if it's cannot be put into
shouldOutput?  With a comment why those should be skipped?

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
File lib/compiler/implementation/dart_backend/placeholder.dart (right):

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder.dart:9: class
TypePlaceholder extends Placeholder {
looks like all those placeholders can be unified in the simple way: they just
all the elements which should be renamed later.  TypePlaceholder will require
type -> type.element substitution and NullPlaceholder will turn into null
element.

Can it simplify our code and get rid of Placeholder abstraction?

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder.dart:14: }
nit: a blank line after class declaration here and below.

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
File lib/compiler/implementation/dart_backend/placeholder_collector.dart
(right):

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:18: //
element === null means dynamic property access.
what !isTopLevel() means?

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:24:
collector.placeholderForElement(node.selector, element);
nit: placeholderForElement makes me think it should return something and it's
weird to see the result is discarded.

If you just want to update internal structures, maybe give it some other name:
makeElementPlaceholder/markElementPlaceholder/registerElementPlaceholder?

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:54: void
processFunction(FunctionElement element, Node node) {
nit: process (here and in the method below) is too generic, can we give it more
precise meaning?  As an idea: process -> collect (fits well
PlaceholderCollector), processFunction ->
{make,mark,registr}FunctionDeclarationPlaceholder?

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:107: }
nit: blank lines between methods, please

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:126:
visitNode(Node node) { node.visitChildren(this); }  // We must go deeper.
memegen link? :)

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:134:
send.visitChildren(this);
super.visitSend(send)?

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:145: final
isPrefixed = element.lookupConstructor(
nit: please, rename isPrefixed -> hasPrefix

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:151:
visit(node.typeArguments);
super.visitTypeAnnotation(node)?

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
File lib/compiler/implementation/dart_backend/renamer.dart (right):

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/dart_backend/renamer.dart:24: // Renamer impl.
nit: please, no abbreviations: Renamer implementation

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/dart_backend/renamer.dart:27: if (placeholder !==
null) return placeholder.rename(this);
nit: could probably fit single line: return placeholder !== null ?
placeholder.rename(this) : null;

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/dart_backend/renamer.dart:35: if (element ===
compiler.types.voidType.element) return null;
may this go away now?

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
File lib/compiler/implementation/tree/unparser.dart (right):

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/tree/unparser.dart:9: Renamer rename;
why it's not final any more?

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/tree/unparser.dart:133: if (node.name !== null)
visit(node.name);
no need in !== null check, but after rebase this diff should go away.

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/tree/unparser.dart:321: visit(node.typeName);
what about TODO as was discussed before?

[Roman, 2012-08-09 05:02:35]

I merged your latest change on privates into this CL too. Unfortunately I could
not make private processing on top level, I can provide more details.
And because of that I reverted processing of TypeAnnotation to visit
typeArguments only instead of calling super.visitNode, otherwise we can
accidentally rename type identifier inside of it.

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
File lib/compiler/implementation/dart_backend/backend.dart (right):

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/dart_backend/backend.dart:76: || element is
SynthesizedConstructorElement
On 2012/08/08 08:27:20, Anton Mukhin wrote:
> can we move this logic into collector than if it's cannot be put into
> shouldOutput?  With a comment why those should be skipped?

Done.

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
File lib/compiler/implementation/dart_backend/placeholder.dart (right):

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder.dart:9: class
TypePlaceholder extends Placeholder {
On 2012/08/08 08:27:20, Anton Mukhin wrote:
> looks like all those placeholders can be unified in the simple way: they just
> all the elements which should be renamed later.  TypePlaceholder will require
> type -> type.element substitution and NullPlaceholder will turn into null
> element.
> 
> Can it simplify our code and get rid of Placeholder abstraction?

Later placeholders will also have scope information, so I would like to keep
them. I can unify method, element and type placeholders at this point.

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder.dart:14: }
On 2012/08/08 08:27:20, Anton Mukhin wrote:
> nit: a blank line after class declaration here and below.

Done.

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
File lib/compiler/implementation/dart_backend/placeholder_collector.dart
(right):

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:18: //
element === null means dynamic property access.
On 2012/08/08 08:27:20, Anton Mukhin wrote:
> what !isTopLevel() means?

added comment that we don't want to rename non-top-level element access.

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:24:
collector.placeholderForElement(node.selector, element);
On 2012/08/08 08:27:20, Anton Mukhin wrote:
> nit: placeholderForElement makes me think it should return something and it's
> weird to see the result is discarded.
> 
> If you just want to update internal structures, maybe give it some other name:
> makeElementPlaceholder/markElementPlaceholder/registerElementPlaceholder?

Done.

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:54: void
processFunction(FunctionElement element, Node node) {
On 2012/08/08 08:27:20, Anton Mukhin wrote:
> nit: process (here and in the method below) is too generic, can we give it
more
> precise meaning?  As an idea: process -> collect (fits well
> PlaceholderCollector), processFunction ->
> {make,mark,registr}FunctionDeclarationPlaceholder?

Done.

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:107: }
On 2012/08/08 08:27:20, Anton Mukhin wrote:
> nit: blank lines between methods, please

Done.

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:134:
send.visitChildren(this);
On 2012/08/08 08:27:20, Anton Mukhin wrote:
> super.visitSend(send)?

Done.

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:145: final
isPrefixed = element.lookupConstructor(
On 2012/08/08 08:27:20, Anton Mukhin wrote:
> nit: please, rename isPrefixed -> hasPrefix

Done.

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:151:
visit(node.typeArguments);
On 2012/08/08 08:27:20, Anton Mukhin wrote:
> super.visitTypeAnnotation(node)?

Done.

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
File lib/compiler/implementation/dart_backend/renamer.dart (right):

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/dart_backend/renamer.dart:24: // Renamer impl.
On 2012/08/08 08:27:20, Anton Mukhin wrote:
> nit: please, no abbreviations: Renamer implementation

Done.

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/dart_backend/renamer.dart:27: if (placeholder !==
null) return placeholder.rename(this);
On 2012/08/08 08:27:20, Anton Mukhin wrote:
> nit: could probably fit single line: return placeholder !== null ?
> placeholder.rename(this) : null;

Done.

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/dart_backend/renamer.dart:35: if (element ===
compiler.types.voidType.element) return null;
On 2012/08/08 08:27:20, Anton Mukhin wrote:
> may this go away now?

Yes! removed

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
File lib/compiler/implementation/tree/unparser.dart (right):

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/tree/unparser.dart:9: Renamer rename;
On 2012/08/08 08:27:20, Anton Mukhin wrote:
> why it's not final any more?

Done.

https://chromiumcodereview.appspot.com/10836128/diff/3003/lib/compiler/implem...
lib/compiler/implementation/tree/unparser.dart:321: visit(node.typeName);
On 2012/08/08 08:27:20, Anton Mukhin wrote:
> what about TODO as was discussed before?

As discussed, keeping as is.

[Anton Muhin, 2012-08-09 06:34:52]

lgtm once we resolve the issue w/ privates.  Nice work!

https://chromiumcodereview.appspot.com/10836128/diff/5012/lib/compiler/implem...
File lib/compiler/implementation/dart_backend/placeholder.dart (right):

https://chromiumcodereview.appspot.com/10836128/diff/5012/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder.dart:20: String toString()
=> 'private[node($node), library($library)]';
nit: not sure if you really need this node/library markers, I believe elements
toString has this information already.  You may want to add 'placeholder' bit
though.

https://chromiumcodereview.appspot.com/10836128/diff/5012/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder.dart:27: String toString()
=> 'element[element($element)]';
ditto.

https://chromiumcodereview.appspot.com/10836128/diff/5012/lib/compiler/implem...
File lib/compiler/implementation/dart_backend/placeholder_collector.dart
(right):

https://chromiumcodereview.appspot.com/10836128/diff/5012/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:15: String
visitDynamicSend(Send node) => tryRenamePrivateId(node);
nit: this method doesn't return String any more, please, remove return type and
do not use => syntax.

BTW, why haven't you got a warning when running unparser test in checked mode?

https://chromiumcodereview.appspot.com/10836128/diff/5012/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:17: String
tryRenamePrivateId(Send node) {
ditto for return type.

https://chromiumcodereview.appspot.com/10836128/diff/5012/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:178:
visit(node.typeArguments);
super.visit?

[Roman, 2012-08-09 07:34:03]

https://chromiumcodereview.appspot.com/10836128/diff/5012/lib/compiler/implem...
File lib/compiler/implementation/dart_backend/placeholder.dart (right):

https://chromiumcodereview.appspot.com/10836128/diff/5012/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder.dart:20: String toString()
=> 'private[node($node), library($library)]';
On 2012/08/09 06:34:52, Anton Mukhin wrote:
> nit: not sure if you really need this node/library markers, I believe elements
> toString has this information already.  You may want to add 'placeholder' bit
> though.

This is just for debugging when I want to print out a placeholder.
Node.toString() just unparses it, no "node" string. Removed "library" and
"element" markers

https://chromiumcodereview.appspot.com/10836128/diff/5012/lib/compiler/implem...
File lib/compiler/implementation/dart_backend/placeholder_collector.dart
(right):

https://chromiumcodereview.appspot.com/10836128/diff/5012/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:15: String
visitDynamicSend(Send node) => tryRenamePrivateId(node);
On 2012/08/09 06:34:52, Anton Mukhin wrote:
> nit: this method doesn't return String any more, please, remove return type
and
> do not use => syntax.
> 
> BTW, why haven't you got a warning when running unparser test in checked mode?

Done. I don't know why, probably returning nothing is not a problem.

https://chromiumcodereview.appspot.com/10836128/diff/5012/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:17: String
tryRenamePrivateId(Send node) {
On 2012/08/09 06:34:52, Anton Mukhin wrote:
> ditto for return type.

Done.

https://chromiumcodereview.appspot.com/10836128/diff/5012/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:178:
visit(node.typeArguments);
On 2012/08/09 06:34:52, Anton Mukhin wrote:
> super.visit?

I can't! If I use super.visit, we will get into identifier inside type
annotation and may rename top level class.