Rename MirrorSystem.getName calls in dart2dart.

Rename MirrorSystem.getName calls in dart2dart.

R=ahe@google.com

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

[ahe, 2013-07-31 13:16:28]

This is a great start!

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

https://codereview.chromium.org/21339002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:448: return
compiler.libraries.values.contains(compiler.mirrorsLibrary);
This would have the same effect and be a little faster:

return compiler.mirrorSystemClass != null;

https://codereview.chromium.org/21339002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:460:
if(mirrorHelping && mirrorLibraryIsInUse()) {
Add space after if.

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

https://codereview.chromium.org/21339002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/dart_backend/mirror_collector.dart:20:
_addMirrorHelperImport();
Please don't use library privacy. It doesn't work.

https://codereview.chromium.org/21339002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/dart_backend/mirror_collector.dart:24:
Uri _mirrorHelperUri = new Uri(path: MIRROR_HELPER_LIBRARY_NAME);
Why is this variable starting with an underscore?

https://codereview.chromium.org/21339002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/dart_backend/mirror_collector.dart:25:
LibraryElement mirrorHelperLib = new LibraryElementX(
This is probably going to cause problems later on. Let's find a better way to do
this.

https://codereview.chromium.org/21339002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/dart_backend/mirror_collector.dart:35:
elementNode.accept(this);
This is somewhat unfortunate.  It is a standard thing to do, and probably what
is taught in compiler classes all over the world: structure your compiler in
separate phases.

Unfortunately, it means that you have to traverse the AST a second time to do a
small amount of work. That can be very expensive.

So it would be better if this could be integrated with an existing visitor.

https://codereview.chromium.org/21339002/diff/1/tests/compiler/dart2js/mirror...
File tests/compiler/dart2js/mirror_helper_test.dart (right):

https://codereview.chromium.org/21339002/diff/1/tests/compiler/dart2js/mirror...
tests/compiler/dart2js/mirror_helper_test.dart:21: Assert elementAsts is as
expected
Precisely what do you mean by "expected"?

[zarah, 2013-08-02 07:53:41]

PTAL

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

https://codereview.chromium.org/21339002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:448: return
compiler.libraries.values.contains(compiler.mirrorsLibrary);
On 2013/07/31 13:16:28, ahe wrote:
> This would have the same effect and be a little faster:
> 
> return compiler.mirrorSystemClass != null;

Done.

https://codereview.chromium.org/21339002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:460:
if(mirrorHelping && mirrorLibraryIsInUse()) {
On 2013/07/31 13:16:28, ahe wrote:
> Add space after if.

Done.

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

https://codereview.chromium.org/21339002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/dart_backend/mirror_collector.dart:20:
_addMirrorHelperImport();
On 2013/07/31 13:16:28, ahe wrote:
> Please don't use library privacy. It doesn't work.

Done.

https://codereview.chromium.org/21339002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/dart_backend/mirror_collector.dart:24:
Uri _mirrorHelperUri = new Uri(path: MIRROR_HELPER_LIBRARY_NAME);
On 2013/07/31 13:16:28, ahe wrote:
> Why is this variable starting with an underscore?

Done.

https://codereview.chromium.org/21339002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/dart_backend/mirror_collector.dart:25:
LibraryElement mirrorHelperLib = new LibraryElementX(
On 2013/07/31 13:16:28, ahe wrote:
> This is probably going to cause problems later on. Let's find a better way to
do
> this.

Added a TODO. When the mirror helper library is constructed we can just load it
directly.

https://codereview.chromium.org/21339002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/dart_backend/mirror_collector.dart:35:
elementNode.accept(this);
On 2013/07/31 13:16:28, ahe wrote:
> This is somewhat unfortunate.  It is a standard thing to do, and probably what
> is taught in compiler classes all over the world: structure your compiler in
> separate phases.
> 
> Unfortunately, it means that you have to traverse the AST a second time to do
a
> small amount of work. That can be very expensive.
> 
> So it would be better if this could be integrated with an existing visitor.

Done.

https://codereview.chromium.org/21339002/diff/1/tests/compiler/dart2js/mirror...
File tests/compiler/dart2js/mirror_helper_test.dart (right):

https://codereview.chromium.org/21339002/diff/1/tests/compiler/dart2js/mirror...
tests/compiler/dart2js/mirror_helper_test.dart:21: Assert elementAsts is as
expected
On 2013/07/31 13:16:28, ahe wrote:
> Precisely what do you mean by "expected"?

Added tests.

[ahe, 2013-08-06 15:53:36]

Nice. Mostly nits below.

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

https://codereview.chromium.org/21339002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:129: bool
mirrorHelping = false;
Could you make the name a little more descriptive?
Also, mention that it is set by the test.

https://codereview.chromium.org/21339002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:486: 
Extra line.

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

https://codereview.chromium.org/21339002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/dart_backend/placeholder_collector.dart:109:
compiler.backend.registerStaticSend(element, node);
Since this is already place in a backend, I don't think you need to go through
the backend interface.

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

https://codereview.chromium.org/21339002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirror_renamer/mirror_renamer.dart:7:
import '../elements/modelx.dart' show LibraryElementX;
Add TODO to remove this.

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

https://codereview.chromium.org/21339002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:18:
'$MIRROR_HELPER_LIBRARY_PREFIX.$MIRROR_HELPER_CLASS';
This could be a constant as well.

https://codereview.chromium.org/21339002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:27: new
Script(mirrorHelperUri, null));
I'm really not comfortable that you instantiate LibraryElementX.  Please add
TODOs that explicitly mentions that creating a LibraryElementX outside the
library loader is really bad.

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

https://codereview.chromium.org/21339002/diff/7001/tests/compiler/dart2js/mir...
tests/compiler/dart2js/mirror_helper_test.dart:26: const String
MIRROR_HELPER_CLASS = 'MirrorHelper';
It's nice that you define constants, but I feel it would be even nicer if you
didn't define them twice :-)

https://codereview.chromium.org/21339002/diff/7001/tests/compiler/dart2js/mir...
tests/compiler/dart2js/mirror_helper_test.dart:44: backend.mirrorHelping = true;
I think these four lines are almost always the same. I suggest adding a method
for that (with two named bool parameters to indicate the variable parts).

https://codereview.chromium.org/21339002/diff/7001/tests/compiler/dart2js/mir...
tests/compiler/dart2js/mirror_helper_test.dart:50:
backend.memberNodes.values.first.first.body.statements.nodes.head;
This feels a bit brittle to me. How about (not tested):

ClassElement cls = compiler.mainApp.find(const SourceString('Foo'));
FunctionExpression function = cls.localLookup(const
SourceString('noSuchMethod')).parseNode(compiler);
Send getNameFunctionNode = function.body.statements.nodes.toList().single;

https://codereview.chromium.org/21339002/diff/7001/tests/compiler/dart2js/mir...
tests/compiler/dart2js/mirror_helper_test.dart:59: }
My comments above apply to the following functions as well.

[zarah, 2013-08-08 12:17:10]

PTAL

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

https://codereview.chromium.org/21339002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:129: bool
mirrorHelping = false;
On 2013/08/06 15:53:36, ahe wrote:
> Could you make the name a little more descriptive?
> Also, mention that it is set by the test.

Done.

https://codereview.chromium.org/21339002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:486: 
On 2013/08/06 15:53:36, ahe wrote:
> Extra line.

Done.

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

https://codereview.chromium.org/21339002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/dart_backend/placeholder_collector.dart:109:
compiler.backend.registerStaticSend(element, node);
On 2013/08/06 15:53:36, ahe wrote:
> Since this is already place in a backend, I don't think you need to go through
> the backend interface.

Done.

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

https://codereview.chromium.org/21339002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirror_renamer/mirror_renamer.dart:7:
import '../elements/modelx.dart' show LibraryElementX;
On 2013/08/06 15:53:36, ahe wrote:
> Add TODO to remove this.

Done.

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

https://codereview.chromium.org/21339002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:18:
'$MIRROR_HELPER_LIBRARY_PREFIX.$MIRROR_HELPER_CLASS';
On 2013/08/06 15:53:36, ahe wrote:
> This could be a constant as well.

Done.

https://codereview.chromium.org/21339002/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:27: new
Script(mirrorHelperUri, null));
On 2013/08/06 15:53:36, ahe wrote:
> I'm really not comfortable that you instantiate LibraryElementX.  Please add
> TODOs that explicitly mentions that creating a LibraryElementX outside the
> library loader is really bad.

Done.

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

https://codereview.chromium.org/21339002/diff/7001/tests/compiler/dart2js/mir...
tests/compiler/dart2js/mirror_helper_test.dart:26: const String
MIRROR_HELPER_CLASS = 'MirrorHelper';
On 2013/08/06 15:53:36, ahe wrote:
> It's nice that you define constants, but I feel it would be even nicer if you
> didn't define them twice :-)

Removed! :-)

https://codereview.chromium.org/21339002/diff/7001/tests/compiler/dart2js/mir...
tests/compiler/dart2js/mirror_helper_test.dart:44: backend.mirrorHelping = true;
On 2013/08/06 15:53:36, ahe wrote:
> I think these four lines are almost always the same. I suggest adding a method
> for that (with two named bool parameters to indicate the variable parts).

Done.

https://codereview.chromium.org/21339002/diff/7001/tests/compiler/dart2js/mir...
tests/compiler/dart2js/mirror_helper_test.dart:50:
backend.memberNodes.values.first.first.body.statements.nodes.head;
On 2013/08/06 15:53:36, ahe wrote:
> This feels a bit brittle to me. How about (not tested):
> 
> ClassElement cls = compiler.mainApp.find(const SourceString('Foo'));
> FunctionExpression function = cls.localLookup(const
> SourceString('noSuchMethod')).parseNode(compiler);
> Send getNameFunctionNode = function.body.statements.nodes.toList().single;

I agree. The point is however, that in the backend the AST is cloned. Hence, the
node retrieved in your suggestion is not the same as the one in the renames map
and so it doesn't work.

https://codereview.chromium.org/21339002/diff/7001/tests/compiler/dart2js/mir...
tests/compiler/dart2js/mirror_helper_test.dart:59: }
On 2013/08/06 15:53:36, ahe wrote:
> My comments above apply to the following functions as well.

Done.

[ahe, 2013-08-08 12:24:06]

LGTM!

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

https://codereview.chromium.org/21339002/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/mirror_renamer/mirror_renamer.dart:7:
//TODO(zarah) Remove this hack! LibraryElementX should not be created outside
Add space after //. Add colon after ).

https://codereview.chromium.org/21339002/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/mirror_renamer/mirror_renamer.dart:8:
//            the library loader!
Only one space.

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

https://codereview.chromium.org/21339002/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:25:
//TODO(zarah) Remove this hack! LibraryElementX should not be created
Add space after // and add a colon.

https://codereview.chromium.org/21339002/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:26: //    
       outside the library loader. When actual mirror helper library
Only one space.

https://codereview.chromium.org/21339002/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:27: //    
       is created, change to load that.
Ditto.

[zarah, 2013-08-08 12:53:35]

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

https://codereview.chromium.org/21339002/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/mirror_renamer/mirror_renamer.dart:7:
//TODO(zarah) Remove this hack! LibraryElementX should not be created outside
On 2013/08/08 12:24:06, ahe wrote:
> Add space after //. Add colon after ).

Done.

https://codereview.chromium.org/21339002/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/mirror_renamer/mirror_renamer.dart:8:
//            the library loader!
On 2013/08/08 12:24:06, ahe wrote:
> Only one space.

Done.

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

https://codereview.chromium.org/21339002/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:25:
//TODO(zarah) Remove this hack! LibraryElementX should not be created
On 2013/08/08 12:24:06, ahe wrote:
> Add space after // and add a colon.

Done.

https://codereview.chromium.org/21339002/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:26: //    
       outside the library loader. When actual mirror helper library
On 2013/08/08 12:24:06, ahe wrote:
> Only one space.

Done.

https://codereview.chromium.org/21339002/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/mirror_renamer/renamer.dart:27: //    
       is created, change to load that.
On 2013/08/08 12:24:06, ahe wrote:
> Ditto.

Done.

[zarah, 2013-08-08 12:59:53]

Committed patchset #4 manually as r25924 (presubmit successful).