Add JavaScript AST.

Add JavaScript AST.

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

[kasperl, 2012-08-07 07:46:45]

DBC:

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/js/nodes.dart (right):

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:157: Dynamic sourcePosition;
var

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:199: class NOP extends Statement {
I would call this EmptyStatement. I don't like the abbreviation.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:229: Expression test;
test -> condition?

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:230: Expression incr;
incr -> update?

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:243: Expression lhs;
I would try to cut down on the abbreviations (if possible).

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:278: String id;  // Can be null.
Can more of these fields be final? Seems like that should work just fine in
almost all cases.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:339: class With extends Statement {
Do we need this?

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:434: int get precedenceLevel() =>
EXPRESSION;
Put this in Expression?

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:469: class Vassign extends Assign {
Vassign? Maybe think of a better name.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:493: class VassignOp extends Vassign {
Vassign.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:508: class Accsign extends Assign {
Accsign?

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:711: class Fun extends Expression {
Function?

[Lasse Reichstein Nielsen, 2012-08-08 11:16:38]

LGTM with more finals and far fewer abbreviations.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/js/nodes.dart (right):

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:30: T
visitExpressionBlob(ExpressionBlob blob);
LiteralExpression.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:33: T visitVassign(Vassign vassign);
Expand name. What's a "Vassign"?

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:34: T visitInit(Init init);
Initialize? Initializer? Initialization?

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:35: T visitAccsign(Accsign accsign);
Wut?

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:36: T visitVassignOp(VassignOp
vassignOp);
VariableAssignOperation? Operator?

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:41: T visitBinary(Binary binary);
Perhaps BinaryOperation? But not necessarily, just "binary" does make sense.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:45: T visitRef(Ref ref);
Wut?

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:47: T visitDecl(Decl decl);
Declaration.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:48: T visitParam(Param param);
Parameter ... you get the idea. Don't abbreviate names.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:49: T visitAccess(Access access);
What is an "Access"? Better name?

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:54: T visitBoolLiteral(BoolLiteral
node);
I prefer "LiteralBool", but that's subjective.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:60: T visitArrayElement(ArrayElement
node);
What is an ArrayElement?

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:87: T visitContinue(Continue node) =>
visitInterruption(node);
I'd prefer visitControlFlow or visitJump. Interrupts are something different.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:157: Dynamic sourcePosition;
Or something better, if possible.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:181: List<Statement> elements;
elements -> statements?

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:192: Expression expr;
Don't abbreviate.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:199: class NOP extends Statement {
+1

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:207: Node test;
"Expression test;" ?

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:211: If.then(this.test, this.then) :
this.otherwise = new NOP();
Could you reuse an empty statement, or would it be too horrible that they have
the same id?

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:230: Expression incr;
What Kasper said!

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:243: Expression lhs;
I'd say cut it all the way down to zero, and it better be possible!

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:243: Expression lhs;
The lhs can be a declaration:
 for (var i in beep) ...
so Expression is probably too strict.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:278: String id;  // Can be null.
id -> targetLabel.
Ditto for break.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:295: Return(this.expr);
make this.expr optional.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:299: expr.accept(visitor);
if (expr !== null) expr.accept(visitor);

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:316: Block finallyPart;
Add comment saying that either, but not both, of catchPart and finallyPart can
be null.
Perhaps make a check for it in the constructor.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:400: class Labeled extends Statement {
LabeledStatement (Labeled isn't a noun phrase).

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:401: String id;
id -> label.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:423: class ExpressionBlob extends
Expression {
ExpressionBlob -> LiteralExpression.
blob -> source.
What is "data"? Needs explanation.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:462: abstract Expression get lhs();
Getters below constructors. Only fields above.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:469: class Vassign extends Assign {
No "maybe". Do! I'm guessing "VariableAssignment"

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:493: class VassignOp extends Vassign {
Is this, e.g.,
 v += 42;
?
Is it necessary to have it separate from Vassign?

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:508: class Accsign extends Assign {
What is an Access? I'm *guessing* it's, e.g., x.y. Is it also x['z']?

Why have different Vassign and Accsign classes, and not just one Assign that has
an expression as LHS?

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:550: class New extends Expression {
Could "New" extend "Call"? They have the same fields and precedence, it's just a
"special kind of call"?

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:580: : super(new Ref(op),
<Expression>[left, right]);
That seems wrong. The "op" is not referencing a variable.

Also: new Ref.operator(op)

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:634: Unary(String op, Expression arg)
: super(new Ref(op), <Expression>[arg]);
new Ref.unaryOperator(op) ?

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:676: class Decl extends Ref {
Extends Ref? I thought both referred to a variable, but this way, a Ref seems
just to *be* the variable.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:691: This() : super("this");
Clever.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:779: // [Literal].
Why not?

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:794: int index;
When is that useful?

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:807: List<PropertyInit> properties;
Just "Property". The "Init" makes no sense.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:831: // [Literal].
Why not, it doesn't even need to extend visitChildren?

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/js/precedence.dart (right):

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/precedence.dart:25: final PRIMARY = CALL + 1;
Seems to be missing a "member" precedence, or do you conflate that with CALL?

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/js/printer.dart (right):

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/printer.dart:24: outBuffer.add(str);
You can move the outBuffer into the if too.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/printer.dart:41: void
recordSourcePosition(Dynamic position) {
Dynamic -> var, but please find a type.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/printer.dart:296:
visitNestedExpression(node.decl, PRIMARY,
It should be able to be a declaration too.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/printer.dart:329:
visitNestedExpression(node.expr, EXPRESSION,
That's ASSIGNMENT_EXPRESSION, not EXPRESSION.
You can't have comma-separated expressions here.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/printer.dart:595: if (lastCharCode ==
"+".charCodeAt(0)) out(" ");
If you are going to be this primitive, just store the last char as a string.
Otherwise, define your char codes.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/printer.dart:664: if (isDigit(lastCharCode))
out(" ");
You should be able to parenthesize the number, somehow. Otherwise this AST isn't
buying us much over a single-pass print.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/printer.dart:770: List<String> parts =
blob.split('#');
"blob" is really the wrong name when you interpret the contents. This is, if
anything, a delayed expression template application (do find a short name for
that!)

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:1276: // TODO(floitsch): should
this really be the breakLabelName?
Good question. I really can't remember if there is a special case here.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:2339: js.Ref bailoutTarget;
Not a Ref. Perhaps just make a Label class.

[floitsch, 2012-08-09 16:24:07]

PTAL.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/js/nodes.dart (right):

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:30: T
visitExpressionBlob(ExpressionBlob blob);
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> LiteralExpression.

Done.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:33: T visitVassign(Vassign vassign);
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> Expand name. What's a "Vassign"?

Simplified.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:41: T visitBinary(Binary binary);
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> Perhaps BinaryOperation? But not necessarily, just "binary" does make sense.

Prefer to call it Binary. Otherwise Prefix and Postfix would need "Operation"
too.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:54: T visitBoolLiteral(BoolLiteral
node);
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> I prefer "LiteralBool", but that's subjective.

Done.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:60: T visitArrayElement(ArrayElement
node);
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> What is an ArrayElement?
An element in an ArrayInitializer. Couldn't find a better name.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:87: T visitContinue(Continue node) =>
visitInterruption(node);
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> I'd prefer visitControlFlow or visitJump. Interrupts are something different.

Done.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:157: Dynamic sourcePosition;
On 2012/08/07 07:46:45, kasperl wrote:
> var

Done.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:157: Dynamic sourcePosition;
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> Or something better, if possible.
Not sure that is easy. I don't think the AST should know how the source-position
is encoded.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:181: List<Statement> elements;
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> elements -> statements?

Done.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:192: Expression expr;
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> Don't abbreviate.

Done.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:199: class NOP extends Statement {
On 2012/08/07 07:46:45, kasperl wrote:
> I would call this EmptyStatement. I don't like the abbreviation.

Done.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:207: Node test;
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> "Expression test;" ?

Done.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:211: If.then(this.test, this.then) :
this.otherwise = new NOP();
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> Could you reuse an empty statement, or would it be too horrible that they have
> the same id?

I think of an AST as a tree, and not as a DAG. We can revisit this decision,
though.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:229: Expression test;
On 2012/08/07 07:46:45, kasperl wrote:
> test -> condition?

Done.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:230: Expression incr;
On 2012/08/07 07:46:45, kasperl wrote:
> incr -> update?

Done.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:230: Expression incr;
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> What Kasper said!

Done.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:243: Expression lhs;
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> I'd say cut it all the way down to zero, and it better be possible!

trying :)

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:243: Expression lhs;
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> The lhs can be a declaration:
>  for (var i in beep) ...
> so Expression is probably too strict.

var x is a an expression in this AST.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:243: Expression lhs;
On 2012/08/07 07:46:45, kasperl wrote:
> I would try to cut down on the abbreviations (if possible).

Done.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:278: String id;  // Can be null.
On 2012/08/07 07:46:45, kasperl wrote:
> Can more of these fields be final? Seems like that should work just fine in
> almost all cases.

Done.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:295: Return(this.expr);
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> make this.expr optional.

Done.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:299: expr.accept(visitor);
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> if (expr !== null) expr.accept(visitor);

Done.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:316: Block finallyPart;
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> Add comment saying that either, but not both, of catchPart and finallyPart can
> be null.
> Perhaps make a check for it in the constructor.

Done.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:339: class With extends Statement {
On 2012/08/07 07:46:45, kasperl wrote:
> Do we need this?

Done.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:400: class Labeled extends Statement {
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> LabeledStatement (Labeled isn't a noun phrase).

Done.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:401: String id;
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> id -> label.

Done.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:423: class ExpressionBlob extends
Expression {
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> ExpressionBlob -> LiteralExpression.
> blob -> source.
> What is "data"? Needs explanation.

blob -> template.
data -> inputs.
done.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:434: int get precedenceLevel() =>
EXPRESSION;
On 2012/08/07 07:46:45, kasperl wrote:
> Put this in Expression?

It's not duplicated that often, and I prefer to be forced to think about it at
every Expression.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:462: abstract Expression get lhs();
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> Getters below constructors. Only fields above.

Done.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:469: class Vassign extends Assign {
On 2012/08/07 07:46:45, kasperl wrote:
> Vassign? Maybe think of a better name.

Removed.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:550: class New extends Expression {
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> Could "New" extend "Call"? They have the same fields and precedence, it's just
a
> "special kind of call"?

Nice suggestion. done.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:580: : super(new Ref(op),
<Expression>[left, right]);
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> That seems wrong. The "op" is not referencing a variable.

Depends on how you look at operators. For Schemers and MLers its natural to see
the '+' operator as a reference to the plus-function.
Keeping it to simplify the AST.
> 
> Also: new Ref.operator(op)
done.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:634: Unary(String op, Expression arg)
: super(new Ref(op), <Expression>[arg]);
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> new Ref.unaryOperator(op) ?

Done.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:676: class Decl extends Ref {
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> Extends Ref? I thought both referred to a variable, but this way, a Ref seems
> just to *be* the variable.

As discussed.
Added VariableReference super class.
Ref then became VariableUse and Decl became VariableDeclaration (both inheriting
from VariableReference).

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:711: class Fun extends Expression {
On 2012/08/07 07:46:45, kasperl wrote:
> Function?

This would shadow the global Function type.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:779: // [Literal].
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> Why not?

Changed name to ArrayInitialization.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:794: int index;
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> When is that useful?

added comment. An ArrayInitialization has a sparse list of its elements.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:807: List<PropertyInit> properties;
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> Just "Property". The "Init" makes no sense.

Done.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:831: // [Literal].
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> Why not, it doesn't even need to extend visitChildren?

As discussed. It is evaluated and thus has a side-effect.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/js/precedence.dart (right):

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/precedence.dart:25: final PRIMARY = CALL + 1;
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> Seems to be missing a "member" precedence, or do you conflate that with CALL?

Moved comment from LHS down to CALL.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/js/printer.dart (right):

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/printer.dart:24: outBuffer.add(str);
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> You can move the outBuffer into the if too.

Done.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/printer.dart:41: void
recordSourcePosition(Dynamic position) {
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> Dynamic -> var, but please find a type.

As discussed. I would prefer to keep var here.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/printer.dart:296:
visitNestedExpression(node.decl, PRIMARY,
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> It should be able to be a declaration too.

changed to EXPRESSION.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/printer.dart:329:
visitNestedExpression(node.expr, EXPRESSION,
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> That's ASSIGNMENT_EXPRESSION, not EXPRESSION.
> You can't have comma-separated expressions here.

ignoring comment you sent out unintentionally.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/printer.dart:595: if (lastCharCode ==
"+".charCodeAt(0)) out(" ");
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> If you are going to be this primitive, just store the last char as a string.
> Otherwise, define your char codes.

Importing the charcodes from ../util/characters.dart now.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/printer.dart:664: if (isDigit(lastCharCode))
out(" ");
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> You should be able to parenthesize the number, somehow. Otherwise this AST
isn't
> buying us much over a single-pass print.

As discussed: we can change this later.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/js/printer.dart:770: List<String> parts =
blob.split('#');
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> "blob" is really the wrong name when you interpret the contents. This is, if
> anything, a delayed expression template application (do find a short name for
> that!)

Done.

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/10825180/diff/5001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:2339: js.Ref bailoutTarget;
On 2012/08/08 11:16:38, Lasse Reichstein Nielsen wrote:
> Not a Ref. Perhaps just make a Label class.

It is not a label. It is true, that the isolateBailoutAcces from the namer
returns a PropertyAccess, and is therefore not really a VariableReference, but
until the namer is fixed wrapping it into a VariableReference looks ok to me.

Changed the bailoutTarget assignment in the first to be a PropertyAccess,
though.

[Lasse Reichstein Nielsen, 2012-08-10 11:54:51]

LGTM

https://chromiumcodereview.appspot.com/10825180/diff/18/lib/compiler/implemen...
File lib/compiler/implementation/js/nodes.dart (right):

https://chromiumcodereview.appspot.com/10825180/diff/18/lib/compiler/implemen...
lib/compiler/implementation/js/nodes.dart:139: T
visitObjectLiteral(ObjectInitialization node) => visitExpression(node);
Inconsistent naming.
The ES spec uses "ObjectInitializer" for object literals, you might want that
instead of "initialization".

https://chromiumcodereview.appspot.com/10825180/diff/18/lib/compiler/implemen...
lib/compiler/implementation/js/nodes.dart:157: void visitChildren(NodeVisitor
visitor) {
Consider having an empty line between implemented methods. At least if they are
not all single-line functions.

https://chromiumcodereview.appspot.com/10825180/diff/18/lib/compiler/implemen...
lib/compiler/implementation/js/nodes.dart:524: switch (op) {
For a later version, you could consider doing lookup in a constant Map here
instead of a switch.

[floitsch, 2012-08-10 13:59:01]

https://chromiumcodereview.appspot.com/10825180/diff/18/lib/compiler/implemen...
File lib/compiler/implementation/js/nodes.dart (right):

https://chromiumcodereview.appspot.com/10825180/diff/18/lib/compiler/implemen...
lib/compiler/implementation/js/nodes.dart:139: T
visitObjectLiteral(ObjectInitialization node) => visitExpression(node);
On 2012/08/10 11:54:51, Lasse Reichstein Nielsen wrote:
> Inconsistent naming.
> The ES spec uses "ObjectInitializer" for object literals, you might want that
> instead of "initialization".

Done.

https://chromiumcodereview.appspot.com/10825180/diff/18/lib/compiler/implemen...
lib/compiler/implementation/js/nodes.dart:157: void visitChildren(NodeVisitor
visitor) {
On 2012/08/10 11:54:51, Lasse Reichstein Nielsen wrote:
> Consider having an empty line between implemented methods. At least if they
are
> not all single-line functions.

Done.

https://chromiumcodereview.appspot.com/10825180/diff/18/lib/compiler/implemen...
lib/compiler/implementation/js/nodes.dart:524: switch (op) {
On 2012/08/10 11:54:51, Lasse Reichstein Nielsen wrote:
> For a later version, you could consider doing lookup in a constant Map here
> instead of a switch.

added TODO.

[ngeoffray, 2012-08-16 13:22:15]

https://chromiumcodereview.appspot.com/10825180/diff/2011/lib/compiler/implem...
File lib/compiler/implementation/js/nodes.dart (right):

https://chromiumcodereview.appspot.com/10825180/diff/2011/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:106: T
visitVariableInitialization(VariableInitialization node) {
So should VariableInitialization implement Statement and Expression?

[floitsch, 2012-08-16 14:12:52]

https://chromiumcodereview.appspot.com/10825180/diff/2011/lib/compiler/implem...
File lib/compiler/implementation/js/nodes.dart (right):

https://chromiumcodereview.appspot.com/10825180/diff/2011/lib/compiler/implem...
lib/compiler/implementation/js/nodes.dart:106: T
visitVariableInitialization(VariableInitialization node) {
On 2012/08/16 13:22:15, ngeoffray wrote:
> So should VariableInitialization implement Statement and Expression?
I treat it as an ExpressionStatement. That seems to work fine.