Emit constants using ASTs

Emit constants using ASTs

Cleans up inconsistency reported in Issue 6081.

The main observable difference other than Issue 6081 is that properties in object literals are quoted more intelligently and minify slightly more.

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

[ngeoffray, 2012-11-27 09:26:28]

LGTM! This is very nice. Adding Erik for the minify comment and Florian as he
wrote the original code.

https://codereview.chromium.org/11308175/diff/8001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart
(right):

https://codereview.chromium.org/11308175/diff/8001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart:42:
class ConstantReferenceEmitter implements ConstantVisitor<js.Expression> {
High-level comments on this class please.

https://codereview.chromium.org/11308175/diff/8001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart:95:
if (compiler.enableMinification) {
@erik: Why isn't that being taken care by the minifier code?

https://codereview.chromium.org/11308175/diff/8001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart:155:
class ConstantInitializerEmitter implements ConstantVisitor<js.Expression> {
Comments too.

https://codereview.chromium.org/11308175/diff/8001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/11308175/diff/8001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:411:
js.Expression isolatePropertiesAccessForConstant(String constantName) {
I'd prefer not having this method here. The namer is pretty consistent in
returning strings.

[sra1, 2012-11-28 03:59:44]

I re-submitted as https://codereview.chromium.org/11348270
 after discovering that global '$' was shadowed by the local
'extraArgumentName'.

https://codereview.chromium.org/11308175/diff/8001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart
(right):

https://codereview.chromium.org/11308175/diff/8001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart:42:
class ConstantReferenceEmitter implements ConstantVisitor<js.Expression> {
On 2012/11/27 09:26:28, ngeoffray wrote:
> High-level comments on this class please.

Done.

https://codereview.chromium.org/11308175/diff/8001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart:95:
if (compiler.enableMinification) {
On 2012/11/27 09:26:28, ngeoffray wrote:
> @erik: Why isn't that being taken care by the minifier code?

This transformation is currently done in codegen.
It could be moved to printing, but that would complicate the handling of
precedence.

https://codereview.chromium.org/11308175/diff/8001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart:155:
class ConstantInitializerEmitter implements ConstantVisitor<js.Expression> {
On 2012/11/27 09:26:28, ngeoffray wrote:
> Comments too.

Done.

https://codereview.chromium.org/11308175/diff/8001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/11308175/diff/8001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:411:
js.Expression isolatePropertiesAccessForConstant(String constantName) {
On 2012/11/27 09:26:28, ngeoffray wrote:
> I'd prefer not having this method here. The namer is pretty consistent in
> returning strings.

I might agree, but that leaves the question of where to put this method?
In this case I will fold it into ConstantReferenceEmitter.

isolateAccess will need to be converted to return an expression too, but it is
called from 25 places in 6 files.  Better to have one place that knows how to
'index' the current isolate than 25.  Consider what happens if we wrap the code
in a function - 25 places to update expression from an access path to a simple
local variable.

[ngeoffray, 2012-11-28 09:25:20]

https://codereview.chromium.org/11308175/diff/8001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/11308175/diff/8001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:411:
js.Expression isolatePropertiesAccessForConstant(String constantName) {
On 2012/11/28 03:59:44, sra1 wrote:
> On 2012/11/27 09:26:28, ngeoffray wrote:
> > I'd prefer not having this method here. The namer is pretty consistent in
> > returning strings.
> 
> I might agree, but that leaves the question of where to put this method?
> In this case I will fold it into ConstantReferenceEmitter.
> 
> isolateAccess will need to be converted to return an expression too, but it is
> called from 25 places in 6 files.  Better to have one place that knows how to
> 'index' the current isolate than 25.  Consider what happens if we wrap the
code
> in a function - 25 places to update expression from an access path to a simple
> local variable.

So maybe the isolateAccess, isolateBailoutAccess... methods that do property
access on the isolate should really not be in the namer. We should get a js.Ast
builder class that helps for doing these operations.

[floitsch, 2012-11-28 10:04:31]

https://codereview.chromium.org/11308175/diff/8001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/11308175/diff/8001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:411:
js.Expression isolatePropertiesAccessForConstant(String constantName) {
On 2012/11/28 09:25:21, ngeoffray wrote:
> On 2012/11/28 03:59:44, sra1 wrote:
> > On 2012/11/27 09:26:28, ngeoffray wrote:
> > > I'd prefer not having this method here. The namer is pretty consistent in
> > > returning strings.
> > 
> > I might agree, but that leaves the question of where to put this method?
> > In this case I will fold it into ConstantReferenceEmitter.
> > 
> > isolateAccess will need to be converted to return an expression too, but it
is
> > called from 25 places in 6 files.  Better to have one place that knows how
to
> > 'index' the current isolate than 25.  Consider what happens if we wrap the
> code
> > in a function - 25 places to update expression from an access path to a
simple
> > local variable.
> 
> So maybe the isolateAccess, isolateBailoutAccess... methods that do property
> access on the isolate should really not be in the namer. We should get a
js.Ast
> builder class that helps for doing these operations.

I agree. And it should probably be the emitter anyways that should give that
information.