Build finishClasses from JS AST nodes.

Build "init" from JS AST nodes.

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

[ahe, 2013-02-07 13:48:35]

Stephen, Erik: FYI, no need to have three people reviewing this change.

[ahe, 2013-02-07 14:02:31]

I cannot create a completely empty diff between the generated out of this
version and the original version. However, I have made another CL (which I won't
submit) which shows the minor tweaks necessary to get an empty diff. See:
https://codereview.chromium.org/12212064/

[ahe, 2013-02-07 14:14:51]

Not much, but this saves 913 bytes on swarm in minified mode.

My plan is to get the entire "init" function converted, as I need to change it a
lot to implement deferred/lazy loading, and I'm actually having a really hard
time reading the "init" function as it isn't correctly indented in the output.
So I figured rather than fixing that by adding more bytes (added whitespace), I
should make the function smaller in minified mode :-)

[sra1, 2013-02-07 22:51:28]

I'm finding the bigger functions a little too hard to read.

One of the problems is that, with .addTo(), the construction has shifted from
begin functional to being operational.  Another problem is that the code is
really big.  5x explosion on a 3 line function is much more manageable that 5x
on a 20 line function.

Florian's idea was that we parse the canned JS to generate the AST.
I think this CL exposes the limits of a library of AST construction combinators,
so  I'm in favor of a parsing approach since the bigger functions will be more
maintainable.

Starting on a JS parser sooner has some advantages.

With a little modification to parse placeholders we could parse the JS(...) code
and solve the problem we have when substituting expressions of unknown
precedence.  We have mostly been very lucky here.

Parsing with placeholders also opens the way for templated generation in some
places where we generate code by hand.  I think we would want named placeholders
(#FOO) for bigger templates rather than the positional JS-# placeholders, and
some splicing (#..ARGS) to make that work.

The parser does not have to cover the whole language, e.g. we can omit parsing
'with', always use // style comments, always use \u escapes for quotes in
strings, limit the syntax of numbers, etc.  We can add statements like switch
and do-while as we need them.  Only a subset of the language is on the critical
path.

[ahe, 2013-02-08 14:07:06]

I have fully converted "init" to by built from JS.

There are many open questions about what approach we should take in the future,
but I would really like to have this change landed so I can make progress on
loading classes lazily.

Cheers,
Peter

[ahe, 2013-02-11 19:32:41]

PTAL, I hope it is more readable now.

Cheers,
Peter

[sra1, 2013-02-12 01:08:26]

On 2013/02/11 19:32:41, ahe wrote:
> PTAL, I hope it is more readable now.
> 
> Cheers,
> Peter

I'm still having a hard time reading this.
Can we VC to discuss?

[ngeoffray, 2013-02-12 08:42:41]

I like the new builder. jsAst and js are a bit annoying, maybe use jst and jsb,
but I understand this is not great either.

About the changes from JS blobs -> AST, not sure what's our best option. We will
probably also have some small JS blobs in a new (third) revision of the isolate
library. The first isolate library had quite a lot of JS, and I doubt we can
maintain such JS if it's written in JS ASTs.

But for now, I'm OK with this change.

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
(right):

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:203: //
var needsAccessor = (lastCharCode & $SUFFIX_MASK) >= $FIRST_SUFFIX_CODE;
line too long

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:250: //
prototype["set\$" + accessorName] = new Function("v", setterString);
line too long

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:538:
js['newPrototype'][js['member']].assign(js['prototype'][js['member']])
line too long

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/com...
File
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter_no_eval.dart
(right):

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter_no_eval.dart:78:
jsAst.Expression buildConstructor(String mangledName, List<String> fieldNames) {
line too long

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter_no_eval.dart:79:
return new jsAst.NamedFunction(
Why don't you use JsBuilder here?

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/com...
File
dart/sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart
(right):

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart:158:
List<jsAst.Parameter> stubParameters) {
line too long

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart:182:
[js.use(name), new jsAst.LiteralNumber('$arity')]))));
line too long

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart:426:
new jsAst.Call(new jsAst.Fun([], new jsAst.Block(statements)), [])),
line too long

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:2154: push(new
js.Binary(cmp, typeOf, js.js.string(typeName)));
wat? Maybe there's something nice we can write here and below.

[ngeoffray, 2013-02-12 11:44:12]

LGTM

[erikcorry, 2013-02-12 12:17:09]

I think it's OK with the readability.  It feels quite a lot lighter than adding
a JS parser to the program.

LGTM

[floitsch, 2013-02-13 18:53:13]

DBC.

https://codereview.chromium.org/12208067/diff/10009/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
(left):

https://codereview.chromium.org/12208067/diff/10009/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:308:
'''/* Opera does not support 'getOwnPropertyNames'. Therefore we use
That should be fixed with today's announcement ;)

Seriously, though: we should a TODO(issue-number) to remove work-arounds for
Opera.
There is another work-around at line 249. That one could actually already been
obsolete (since they fixed it more than 6 months ago).

https://codereview.chromium.org/12208067/diff/10009/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
(right):

https://codereview.chromium.org/12208067/diff/10009/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:508:
js['prototype']['__proto__'].assign(js['superConstructor']['prototype']),
80chars.

[ahe, 2013-02-14 15:22:34]

I found a few problems during testing.

Nicolas, could you take another look? Please ignore patch sets 5 and 6. What is
interesting to you are the changes since patch set 4.

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
(right):

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:203: //
var needsAccessor = (lastCharCode & $SUFFIX_MASK) >= $FIRST_SUFFIX_CODE;
On 2013/02/12 08:42:41, ngeoffray wrote:
> line too long

Done.

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:250: //
prototype["set\$" + accessorName] = new Function("v", setterString);
On 2013/02/12 08:42:41, ngeoffray wrote:
> line too long

Done.

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:538:
js['newPrototype'][js['member']].assign(js['prototype'][js['member']])
On 2013/02/12 08:42:41, ngeoffray wrote:
> line too long

Done.

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/com...
File
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter_no_eval.dart
(right):

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter_no_eval.dart:78:
jsAst.Expression buildConstructor(String mangledName, List<String> fieldNames) {
On 2013/02/12 08:42:41, ngeoffray wrote:
> line too long

I'm not changing this file anymore.

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter_no_eval.dart:79:
return new jsAst.NamedFunction(
On 2013/02/12 08:42:41, ngeoffray wrote:
> Why don't you use JsBuilder here?

Ditto.

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/com...
File
dart/sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart
(right):

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart:158:
List<jsAst.Parameter> stubParameters) {
On 2013/02/12 08:42:41, ngeoffray wrote:
> line too long

Done.

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart:182:
[js.use(name), new jsAst.LiteralNumber('$arity')]))));
On 2013/02/12 08:42:41, ngeoffray wrote:
> line too long

Done.

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart:426:
new jsAst.Call(new jsAst.Fun([], new jsAst.Block(statements)), [])),
On 2013/02/12 08:42:41, ngeoffray wrote:
> line too long

Done.

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/12208067/diff/5002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:2154: push(new
js.Binary(cmp, typeOf, js.js.string(typeName)));
On 2013/02/12 08:42:41, ngeoffray wrote:
> wat? Maybe there's something nice we can write here and below.

Done.

https://codereview.chromium.org/12208067/diff/10009/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
(left):

https://codereview.chromium.org/12208067/diff/10009/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:308:
'''/* Opera does not support 'getOwnPropertyNames'. Therefore we use
On 2013/02/13 18:53:14, floitsch wrote:
> That should be fixed with today's announcement ;)
> 
> Seriously, though: we should a TODO(issue-number) to remove work-arounds for
> Opera.
> There is another work-around at line 249. That one could actually already been
> obsolete (since they fixed it more than 6 months ago).

Good point, I'll add todos and file bugs.

[ngeoffray, 2013-02-14 15:59:00]

Still LGTM