dart2dart Preproces placeholders instead of renaming them lazily,

dart2dart Preproces placeholders instead of renaming them lazily,
just form a map <node,string> for future renames.

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

[Anton Muhin, 2012-08-15 12:19:30]

Just two principal questions first.  Detailed review next

https://chromiumcodereview.appspot.com/10836261/diff/1/lib/compiler/implement...
File lib/compiler/implementation/dart_backend/placeholder.dart (right):

https://chromiumcodereview.appspot.com/10836261/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/placeholder.dart:5: interface
Placeholder {}
no interfaces, please.  Esp. empty ones.

https://chromiumcodereview.appspot.com/10836261/diff/1/lib/compiler/implement...
File lib/compiler/implementation/dart_backend/placeholder_collector.dart
(right):

https://chromiumcodereview.appspot.com/10836261/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:76: final
Map<Node, PrivatePlaceholder> privatePlaceholders;
should those be maps from Nodes to placeholders?  Cannot you keep a list of
nodes associated with a placeholder?

[Roman, 2012-08-15 12:43:21]

https://chromiumcodereview.appspot.com/10836261/diff/1/lib/compiler/implement...
File lib/compiler/implementation/dart_backend/placeholder.dart (right):

https://chromiumcodereview.appspot.com/10836261/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/placeholder.dart:5: interface
Placeholder {}
On 2012/08/15 12:19:30, Anton Muhin wrote:
> no interfaces, please.  Esp. empty ones.

Why not?

https://chromiumcodereview.appspot.com/10836261/diff/1/lib/compiler/implement...
File lib/compiler/implementation/dart_backend/placeholder_collector.dart
(right):

https://chromiumcodereview.appspot.com/10836261/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:76: final
Map<Node, PrivatePlaceholder> privatePlaceholders;
On 2012/08/15 12:19:30, Anton Muhin wrote:
> should those be maps from Nodes to placeholders?  Cannot you keep a list of
> nodes associated with a placeholder?

I don't understand. How about the following plan?
1) Null and unresolved placeholders have no info. So I can just keep track of
nodes that need to be nullified or nodes that are unresolved, in 2 sets:
Set<Node> unresolvedNodes;
Set<Node> nullNodes;
In renamer I just map nullNodes to '' and generate uniques for unresolved ones.
2) For nodes that correspond to element, I can have a map from element to a set
of nodes the element represents:
Map<Element, Set<Node>> elementNodes;
in Renamer I just call renameElement once and map all nodes in the set for given
element to rename result.
3) For local nodes we need to have LocalPlaceholder to keep track of original
name and all nodes with that name:
Map<FunctionElement, Set<LocalPlaceholder>> localNodes,
in LocalPlaceholder we keep track of all the nodes with given identifier.
4) For private nodes we will have a map from LibraryELement to list of nodes:
Map<LibraryElement, Set<Node>> privateNodes;
Renaming is straightforward.

So I keep only LocalPlaceholder and remove all else. Does that sound good?

[Roman, 2012-08-15 13:34:33]

PTAL

[Anton Muhin, 2012-08-15 13:45:46]

Neat!  Let's sort out last bits.

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

https://chromiumcodereview.appspot.com/10836261/diff/6001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder.dart:1: // Copyright (c)
2012, the Dart project authors.  Please see the AUTHORS file
time to merge into placeholder_collect.dart?

https://chromiumcodereview.appspot.com/10836261/diff/6001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder.dart:11:
'local_placeholder[nodes($nodes), id($identifier)]';
nit: swap id and nodes to match declaration order.

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

https://chromiumcodereview.appspot.com/10836261/diff/6001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/renamer.dart:9: class
ConflictingRenamer {
Now you don't need ConflictingRenamer, all you really need is a Map<Node,
String>, can we get rid of ConflictingRenamer?

https://chromiumcodereview.appspot.com/10836261/diff/6001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/renamer.dart:32:
placeholderCollector.elementNodes.forEach((Element element, Set<Node> nodes) {
nit: too long string?

[Roman, 2012-08-15 13:59:14]

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

https://chromiumcodereview.appspot.com/10836261/diff/6001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder.dart:1: // Copyright (c)
2012, the Dart project authors.  Please see the AUTHORS file
On 2012/08/15 13:45:46, Anton Muhin wrote:
> time to merge into placeholder_collect.dart?

Done.

https://chromiumcodereview.appspot.com/10836261/diff/6001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/placeholder.dart:11:
'local_placeholder[nodes($nodes), id($identifier)]';
On 2012/08/15 13:45:46, Anton Muhin wrote:
> nit: swap id and nodes to match declaration order.

Done.

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

https://chromiumcodereview.appspot.com/10836261/diff/6001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/renamer.dart:9: class
ConflictingRenamer {
On 2012/08/15 13:45:46, Anton Muhin wrote:
> Now you don't need ConflictingRenamer, all you really need is a Map<Node,
> String>, can we get rid of ConflictingRenamer?

Yes, as discussed, I will in next CL first fix Emitter that uses renameElement
from ConflictingRenamer, and then refactor this class to function.

https://chromiumcodereview.appspot.com/10836261/diff/6001/lib/compiler/implem...
lib/compiler/implementation/dart_backend/renamer.dart:32:
placeholderCollector.elementNodes.forEach((Element element, Set<Node> nodes) {
On 2012/08/15 13:45:46, Anton Muhin wrote:
> nit: too long string?

Done.

[Anton Muhin, 2012-08-15 13:59:55]

lgtm!