Add renames in output when using the mirror helper library.

Add renames in output when using the mirror helper library.

R=johnniwinther@google.com

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

[ahe, 2013-08-12 15:43:21]

https://codereview.chromium.org/22791002/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/dart_backend/emitter.dart
(right):

https://codereview.chromium.org/22791002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/dart_backend/emitter.dart:16: 
Extra line.

https://codereview.chromium.org/22791002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/dart_backend/emitter.dart:17: bool
isTopLevelMain(Node node) =>
In dart2js, we tend to perfer '{ return expr; }' when '=> expr;' doesn't fit on
one line.

https://codereview.chromium.org/22791002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/dart_backend/emitter.dart:17: bool
isTopLevelMain(Node node) =>
I'm not at all sure about this method. What is it for?

https://codereview.chromium.org/22791002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/dart_backend/emitter.dart:18: (node is
FunctionExpression &&
FunctionExpression functionExpression = node.asFunctionExpression();
if (functionExpression != null) {
...
}

https://codereview.chromium.org/22791002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/dart_backend/emitter.dart:19: (node as
FunctionExpression).name.asIdentifier().source.
....source == const SourceString('main')

https://codereview.chromium.org/22791002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/dart_backend/emitter.dart:24:
(unparser as MirrorRenamerUnparser).unparseTopLevelMain(node);
Don't use casts, they are slow and unnecessary. Instead introduce a new
variable:

MirrorRenamerUnparser mirrorUnparser = unparser;

https://codereview.chromium.org/22791002/diff/1/sdk/lib/_internal/compiler/im...
File
sdk/lib/_internal/compiler/implementation/mirror_renamer/emitter_unparser.dart
(right):

https://codereview.chromium.org/22791002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/mirror_renamer/emitter_unparser.dart:1:
part of mirror_renamer;
No copyright.

[zarah, 2013-08-19 13:42:51]

PTAL

[ahe, 2013-08-19 15:34:06]

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:397: LibraryElement
mirrorHelperLibrary;
Could you move this to the backend?

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:397: LibraryElement
mirrorHelperLibrary;
Document this field, see [mirrorsLibrary].

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:451: FunctionElement
mirrorHelperFunction;
Could this also be moved to the backend?

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:451: FunctionElement
mirrorHelperFunction;
More descriptive name and documentation. See [symbolConstructor].

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart
(right):

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:276: return
(!identical(element.kind, ElementKind.VOID)
Since you're changing this line, would you mind updating it to:

element.kind != ElementKind.VOID

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:409:
useMirrorHelperLibrary = true;
I would store the old value. It is always good when building a compiler that
might call its methods recursively as it gets more advanced. Also, it would
allow you to write this (which I find simpler):

bool oldUseHelper = useMirrorHelperLibrary;
useMirrorHelperLibrary = useMirrorHelperLibrary && element.getLibrary() ==
compiler.mirrorHelperLibrary;
collector.collect(element)
useMirrorHelperLibrary = oldUseHelper;

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:503: void
registerStaticSend(Element element, Node node) {
Add newline before this method.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:519: }
Indentation.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
File
sdk/lib/_internal/compiler/implementation/dart_backend/placeholder_collector.dart
(right):

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/dart_backend/placeholder_collector.dart:110:
collector.backend.registerStaticSend(element, node);
I don't understand this change.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart
(right):

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:12: static
Map<String, String> inverseRenames = new Map<String, String>();
Inverse of what?

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:12: static
Map<String, String> inverseRenames = new Map<String, String>();
No static state please.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:13: static
List<Node> mirrorSystemGetNameNodes = <Node>[];
Document.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:14: static
FunctionExpression mirrorHelperFunctionNode;
Name too general?

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:30: static
void addRenames(Map<Node, String> renames, List<Node> topLevelNodes,
Document.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:41: //Add
toplevel map containing all renames.
Add space after //.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:43: if
(node is Identifier) {
Identifier identifier = node.asIdentifier();
if (node != null) {
  var stringValue = identifier.source.stringValue;
}

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:45: if
(stringValue == MIRROR_HELPER_SYMBOLS_MAP_NAME) {
Accessing stringValue in general won't work. See ByteString in
scanner/byte_strings.dart. This should be written like this:

identifier.source == const SourceString('....')

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:47: } else
if (stringValue != renames[node]) {
Could you put SourceStrings in the 'renames' map?

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:67:
//Replace calls to Mirrorsystem.getName with calls to helper function.
Add space after //.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/scanner/parser_task.dart (right):

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/scanner/parser_task.dart:19:
parser.parseStatement(token);
parseCompilationUnit?

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/scanner/parser_task.dart:25: 
Extra line.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/lib/mirr...
File sdk/lib/_internal/lib/mirror_helper.dart (right):

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/lib/mirr...
sdk/lib/_internal/lib/mirror_helper.dart:4: 
Document this library.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/lib/mirr...
sdk/lib/_internal/lib/mirror_helper.dart:11: const Map _SYMBOLS = null;
Add empty line here.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/lib/mirr...
sdk/lib/_internal/lib/mirror_helper.dart:12: String helperGetName(Symbol sym) {
Please document this method. In particular that it is called from generated Dart
code.

https://codereview.chromium.org/22791002/diff/7001/tests/compiler/dart2js/mir...
File tests/compiler/dart2js/mirror_helper2_test.dart (right):

https://codereview.chromium.org/22791002/diff/7001/tests/compiler/dart2js/mir...
tests/compiler/dart2js/mirror_helper2_test.dart:1: // Copyright (c) 2013, the
Dart project authors.  Please see the AUTHORS file
Could you come up with a less generic name for this test?

https://codereview.chromium.org/22791002/diff/7001/tests/compiler/dart2js/mir...
tests/compiler/dart2js/mirror_helper2_test.dart:75: 
Extra line.

[zarah, 2013-08-20 14:08:17]

PTAL

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:397: LibraryElement
mirrorHelperLibrary;
On 2013/08/19 15:34:06, ahe wrote:
> Document this field, see [mirrorsLibrary].

Done.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:397: LibraryElement
mirrorHelperLibrary;
On 2013/08/19 15:34:06, ahe wrote:
> Could you move this to the backend?

Done.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:451: FunctionElement
mirrorHelperFunction;
On 2013/08/19 15:34:06, ahe wrote:
> Could this also be moved to the backend?

Done.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:451: FunctionElement
mirrorHelperFunction;
On 2013/08/19 15:34:06, ahe wrote:
> More descriptive name and documentation. See [symbolConstructor].

Done.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart
(right):

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:276: return
(!identical(element.kind, ElementKind.VOID)
On 2013/08/19 15:34:06, ahe wrote:
> Since you're changing this line, would you mind updating it to:
> 
> element.kind != ElementKind.VOID

Done.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:409:
useMirrorHelperLibrary = true;
On 2013/08/19 15:34:06, ahe wrote:
> I would store the old value. It is always good when building a compiler that
> might call its methods recursively as it gets more advanced. Also, it would
> allow you to write this (which I find simpler):
> 
> bool oldUseHelper = useMirrorHelperLibrary;
> useMirrorHelperLibrary = useMirrorHelperLibrary && element.getLibrary() ==
> compiler.mirrorHelperLibrary;
> collector.collect(element)
> useMirrorHelperLibrary = oldUseHelper;

Done.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:503: void
registerStaticSend(Element element, Node node) {
On 2013/08/19 15:34:06, ahe wrote:
> Add newline before this method.

Done.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:519: }
On 2013/08/19 15:34:06, ahe wrote:
> Indentation.

Done.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
File
sdk/lib/_internal/compiler/implementation/dart_backend/placeholder_collector.dart
(right):

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/dart_backend/placeholder_collector.dart:110:
collector.backend.registerStaticSend(element, node);
On 2013/08/19 15:34:06, ahe wrote:
> I don't understand this change.

Since the backend is now needed in the collector as well as the SendVisitor it
seemed superfluous to have a backend getter both places, so I moved it into the
collecor, but apparently forgot to delete it in the SendVisitor. This is now
done.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart
(right):

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:12: static
Map<String, String> inverseRenames = new Map<String, String>();
On 2013/08/19 15:34:06, ahe wrote:
> No static state please.

Done.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:12: static
Map<String, String> inverseRenames = new Map<String, String>();
On 2013/08/19 15:34:06, ahe wrote:
> Inverse of what?

Inverse of the renames. Changed the name to symbols, so that it is consistent
with the name in _mirror_helper.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:13: static
List<Node> mirrorSystemGetNameNodes = <Node>[];
On 2013/08/19 15:34:06, ahe wrote:
> Document.

Done.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:14: static
FunctionExpression mirrorHelperFunctionNode;
On 2013/08/19 15:34:06, ahe wrote:
> Name too general?

Done.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:30: static
void addRenames(Map<Node, String> renames, List<Node> topLevelNodes,
On 2013/08/19 15:34:06, ahe wrote:
> Document.

Done.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:41: //Add
toplevel map containing all renames.
On 2013/08/19 15:34:06, ahe wrote:
> Add space after //.

Done.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:43: if
(node is Identifier) {
On 2013/08/19 15:34:06, ahe wrote:
> Identifier identifier = node.asIdentifier();
> if (node != null) {
>   var stringValue = identifier.source.stringValue;
> }

Done.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:45: if
(stringValue == MIRROR_HELPER_SYMBOLS_MAP_NAME) {
On 2013/08/19 15:34:06, ahe wrote:
> Accessing stringValue in general won't work. See ByteString in
> scanner/byte_strings.dart. This should be written like this:
> 
> identifier.source == const SourceString('....')

Done.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:47: } else
if (stringValue != renames[node]) {
On 2013/08/19 15:34:06, ahe wrote:
> Could you put SourceStrings in the 'renames' map?

Not really, since it is used in renamePlaceHolders() for dynamically generated
names. However, the inverseRenames map (renamed to 'symbols') can be from String
to SourceString, thereby we save some conversions from SourceString to String,
but loose the small gain of avoiding entries in the map that correspond to
elements that have been 'renamed' to their original name.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:67:
//Replace calls to Mirrorsystem.getName with calls to helper function.
On 2013/08/19 15:34:06, ahe wrote:
> Add space after //.

Done.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/scanner/parser_task.dart (right):

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/scanner/parser_task.dart:19:
parser.parseStatement(token);
On 2013/08/19 15:34:06, ahe wrote:
> parseCompilationUnit?

parseUnit :-)

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/scanner/parser_task.dart:25: 
On 2013/08/19 15:34:06, ahe wrote:
> Extra line.

Done.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/lib/mirr...
File sdk/lib/_internal/lib/mirror_helper.dart (right):

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/lib/mirr...
sdk/lib/_internal/lib/mirror_helper.dart:4: 
On 2013/08/19 15:34:06, ahe wrote:
> Document this library.

Done.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/lib/mirr...
sdk/lib/_internal/lib/mirror_helper.dart:11: const Map _SYMBOLS = null;
On 2013/08/19 15:34:06, ahe wrote:
> Add empty line here.

Done.

https://codereview.chromium.org/22791002/diff/7001/sdk/lib/_internal/lib/mirr...
sdk/lib/_internal/lib/mirror_helper.dart:12: String helperGetName(Symbol sym) {
On 2013/08/19 15:34:06, ahe wrote:
> Please document this method. In particular that it is called from generated
Dart
> code.

Done.

https://codereview.chromium.org/22791002/diff/7001/tests/compiler/dart2js/mir...
File tests/compiler/dart2js/mirror_helper2_test.dart (right):

https://codereview.chromium.org/22791002/diff/7001/tests/compiler/dart2js/mir...
tests/compiler/dart2js/mirror_helper2_test.dart:1: // Copyright (c) 2013, the
Dart project authors.  Please see the AUTHORS file
On 2013/08/19 15:34:06, ahe wrote:
> Could you come up with a less generic name for this test?

Done.

https://codereview.chromium.org/22791002/diff/7001/tests/compiler/dart2js/mir...
tests/compiler/dart2js/mirror_helper2_test.dart:75: 
On 2013/08/19 15:34:06, ahe wrote:
> Extra line.

Done.

[Johnni Winther, 2013-08-23 09:25:19]

https://codereview.chromium.org/22791002/diff/17001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart
(right):

https://codereview.chromium.org/22791002/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:277: if
(useMirrorHelperLibrary && compiler.mirrorsLibrary != null) {
Logical bug here!

https://codereview.chromium.org/22791002/diff/17001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart
(right):

https://codereview.chromium.org/22791002/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:12:
Map<String, SourceString> symbols = new Map<String, SourceString>();
Document this.

https://codereview.chromium.org/22791002/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:13:
List<Node> mirrorSystemGetNameNodes = <Node>[];
Document this.

https://codereview.chromium.org/22791002/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:43: void
addRenames(Map<Node, String> renames, List<Node> topLevelNodes) {
Document the parameters.

https://codereview.chromium.org/22791002/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:58: const
SourceString(MIRROR_HELPER_SYMBOLS_MAP_NAME)) {
Why is this special-cased? And why is it not put in [symbol]? What happens if
the user code has an identifier '_SYMBOLS'?

https://codereview.chromium.org/22791002/diff/17001/tests/compiler/dart2js/da...
File tests/compiler/dart2js/dart_backend_test.dart (right):

https://codereview.chromium.org/22791002/diff/17001/tests/compiler/dart2js/da...
tests/compiler/dart2js/dart_backend_test.dart:458: print(compiler.backend);
Remove debug code. Perhaps change to assert that [compiler.backend] is an
instance of DartBackend.

https://codereview.chromium.org/22791002/diff/17001/tests/compiler/dart2js/mi...
File tests/compiler/dart2js/mirror_helper_rename_test.dart (right):

https://codereview.chromium.org/22791002/diff/17001/tests/compiler/dart2js/mi...
tests/compiler/dart2js/mirror_helper_rename_test.dart:23:
testWithMirrorHeplerLibrary(minify: true);
Hepler -> Helper

https://codereview.chromium.org/22791002/diff/17001/tests/compiler/dart2js/mi...
tests/compiler/dart2js/mirror_helper_rename_test.dart:24:
testWithMirrorHeplerLibrary(minify: false);
Ditto.

https://codereview.chromium.org/22791002/diff/17001/tests/compiler/dart2js/mi...
tests/compiler/dart2js/mirror_helper_rename_test.dart:73: // Referenced twice in
code and twice in renames map.
Why twice? There is only one call to MirrorSystem.getName(), right?

https://codereview.chromium.org/22791002/diff/17001/tests/compiler/dart2js/mi...
tests/compiler/dart2js/mirror_helper_rename_test.dart:96: 
Remove extra empty line.

https://codereview.chromium.org/22791002/diff/17001/tests/compiler/dart2js/mi...
File tests/compiler/dart2js/mirror_helper_test.dart (right):

https://codereview.chromium.org/22791002/diff/17001/tests/compiler/dart2js/mi...
tests/compiler/dart2js/mirror_helper_test.dart:89: 
Remove extra empty line.

[zarah, 2013-08-23 16:16:36]

PTAL

https://codereview.chromium.org/22791002/diff/17001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart
(right):

https://codereview.chromium.org/22791002/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:277: if
(useMirrorHelperLibrary && compiler.mirrorsLibrary != null) {
On 2013/08/23 09:25:19, Johnni Winther wrote:
> Logical bug here!

Done.

https://codereview.chromium.org/22791002/diff/17001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart
(right):

https://codereview.chromium.org/22791002/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:12:
Map<String, SourceString> symbols = new Map<String, SourceString>();
On 2013/08/23 09:25:19, Johnni Winther wrote:
> Document this.

Done.

https://codereview.chromium.org/22791002/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:13:
List<Node> mirrorSystemGetNameNodes = <Node>[];
On 2013/08/23 09:25:19, Johnni Winther wrote:
> Document this.

Done.

https://codereview.chromium.org/22791002/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:43: void
addRenames(Map<Node, String> renames, List<Node> topLevelNodes) {
On 2013/08/23 09:25:19, Johnni Winther wrote:
> Document the parameters.

Done.

https://codereview.chromium.org/22791002/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:58: const
SourceString(MIRROR_HELPER_SYMBOLS_MAP_NAME)) {
On 2013/08/23 09:25:19, Johnni Winther wrote:
> Why is this special-cased? And why is it not put in [symbol]? What happens if
> the user code has an identifier '_SYMBOLS'?

In theory it is not a problem since all source code names will be renamed to the
same mangled name. But for robustness, this is now changed so it compares the
node to the corresponding element.

https://codereview.chromium.org/22791002/diff/17001/tests/compiler/dart2js/da...
File tests/compiler/dart2js/dart_backend_test.dart (right):

https://codereview.chromium.org/22791002/diff/17001/tests/compiler/dart2js/da...
tests/compiler/dart2js/dart_backend_test.dart:458: print(compiler.backend);
On 2013/08/23 09:25:19, Johnni Winther wrote:
> Remove debug code. Perhaps change to assert that [compiler.backend] is an
> instance of DartBackend.

Done.

https://codereview.chromium.org/22791002/diff/17001/tests/compiler/dart2js/mi...
File tests/compiler/dart2js/mirror_helper_rename_test.dart (right):

https://codereview.chromium.org/22791002/diff/17001/tests/compiler/dart2js/mi...
tests/compiler/dart2js/mirror_helper_rename_test.dart:23:
testWithMirrorHeplerLibrary(minify: true);
On 2013/08/23 09:25:19, Johnni Winther wrote:
> Hepler -> Helper

Done.

https://codereview.chromium.org/22791002/diff/17001/tests/compiler/dart2js/mi...
tests/compiler/dart2js/mirror_helper_rename_test.dart:24:
testWithMirrorHeplerLibrary(minify: false);
On 2013/08/23 09:25:19, Johnni Winther wrote:
> Ditto.

Done.

https://codereview.chromium.org/22791002/diff/17001/tests/compiler/dart2js/mi...
tests/compiler/dart2js/mirror_helper_rename_test.dart:73: // Referenced twice in
code and twice in renames map.
On 2013/08/23 09:25:19, Johnni Winther wrote:
> Why twice? There is only one call to MirrorSystem.getName(), right?

The function getNameHelper from _mirror_helper.dart is inlined, so it appears
twice in the output code: once where it is defined and once where it is called.
Updated the comment.

https://codereview.chromium.org/22791002/diff/17001/tests/compiler/dart2js/mi...
tests/compiler/dart2js/mirror_helper_rename_test.dart:96: 
On 2013/08/23 09:25:19, Johnni Winther wrote:
> Remove extra empty line.

Done.

https://codereview.chromium.org/22791002/diff/17001/tests/compiler/dart2js/mi...
File tests/compiler/dart2js/mirror_helper_test.dart (right):

https://codereview.chromium.org/22791002/diff/17001/tests/compiler/dart2js/mi...
tests/compiler/dart2js/mirror_helper_test.dart:89: 
On 2013/08/23 09:25:19, Johnni Winther wrote:
> Remove extra empty line.

Done.

[Johnni Winther, 2013-08-26 07:38:27]

lgtm

https://codereview.chromium.org/22791002/diff/27001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart
(right):

https://codereview.chromium.org/22791002/diff/27001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:66:
Identifier symbolsMapIdentifier =
Assert that [symbolsMapIdentifer] is not null.

[zarah, 2013-08-26 08:21:52]

Committed patchset #5 manually as r26623 (presubmit successful).

[zarah, 2013-08-28 10:19:03]

https://codereview.chromium.org/22791002/diff/27001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart
(right):

https://codereview.chromium.org/22791002/diff/27001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:66:
Identifier symbolsMapIdentifier =
On 2013/08/26 07:38:27, Johnni Winther wrote:
> Assert that [symbolsMapIdentifer] is not null.

Done.