dart2dart backend: AST and unparser for backend.

dart2dart backend: AST and unparser for backend.

Note that the AST and unparser are not yet integrated with the actual code generation.

BUG=
R=kmillikin@google.com

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

[Kevin Millikin (Google), 2014-04-24 13:24:23]

LGTM with a bunch of stylistic comments.

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart
(right):

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart:45: ///
For instance, an expression such as [:super.f():] is represented
I guess we prefer `super.f()`.

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart:69:
static final TypeAnnotation NUM = new TypeAnnotation("num");
I guess the new-school way to name these is with camelCase.

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart:165:
final String label;
It seems annoying that a statement with multiple labels is represented by a nest
of LabeledStatement?  Maybe more annoying than having a list of labels?

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart:195:
final List<Expression> expressions;
Labeled cases are represented by labeling the statements?

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart:205:
final Statement tryBlock;
The grammar has tryBlock, the body of CatchBlock, and finallyBlock as blocks,
not statements.

There's an obvious interpretation here of any other statement as a singleton
block --- but maybe it's worth calling it out in a comment?

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart:231:
class VariableDefinitions extends Statement {
The spec uses variable 'declaration', we should too.

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart:254:
class FunctionStatement extends Statement {
FunctionDeclaration?

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart:383:
/// by [:toString():]. These string are then concatenated and returned.
`toString()`, and likewise below.

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart:478:
/// Expression of form [:e is T:] or [:e as T:].
Also allow 'is!'?  It seems better to allow it.

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart:502:
Increment.postfix(Expression expression, String operator): this(expression,
Break just before the colon (:), indent four spaces.

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart:676: if
(definesVariable(inner))
Braces.  The rule is one can only omit the braces if the body is a single
statement, on the same line as the if, and there is no else.

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart:706: //
  Would there be a significant speedup using a Visitor or a method
Not only a speedup, but I think it would be more readable :)

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart:721:
else if (e is FunctionExpression) {
Else on the same line as the closing brace to the if:

} else if (...) {

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart:1267:
OpenStringChunk chunk = new OpenStringChunk(null, q, getQuoteCost(q));
The definition of getQuoteCost is a long way away.  Make it local to this
function?

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart:1295:
for (int i=0; i<parts.length; i++) {
Spaces around operators = and <, here and below.

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart:1309:
best[j].end(i),
I prefer not to close over i here.  Lift the function out of the loop and pass
in i as endIndex.

[Kevin Millikin (Google), 2014-04-24 13:27:25]

https://codereview.chromium.org/250523003/diff/1/tests/compiler/dart2js/dart_...
File tests/compiler/dart2js/dart_printer_test.dart (right):

https://codereview.chromium.org/250523003/diff/1/tests/compiler/dart2js/dart_...
tests/compiler/dart2js/dart_printer_test.dart:694: checkDeepEqual(baseline,
secondParse);
I guess this should be Expect.isTrue(checkDeepEqual(baseline, secondparse)). 
You can give a second argument that is a string reason.

[asgerf, 2014-04-25 11:52:44]

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart
(right):

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart:45: ///
For instance, an expression such as [:super.f():] is represented
On 2014/04/24 13:24:23, kmillikin wrote:
> I guess we prefer `super.f()`.

Done.

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart:69:
static final TypeAnnotation NUM = new TypeAnnotation("num");
On 2014/04/24 13:24:23, kmillikin wrote:
> I guess the new-school way to name these is with camelCase.

I thought constants were mostly upper case? In any case(!), I think it's a bad
idea to use a name that coincides with the name of a type (e.g. String or num).

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart:165:
final String label;
On 2014/04/24 13:24:23, kmillikin wrote:
> It seems annoying that a statement with multiple labels is represented by a
nest
> of LabeledStatement?  Maybe more annoying than having a list of labels?

I figured the backend would want to merge adjacent labels anyway.

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart:195:
final List<Expression> expressions;
On 2014/04/24 13:24:23, kmillikin wrote:
> Labeled cases are represented by labeling the statements?

Labeled cases are not yet supported. Mainly because I could not figure out how
they work.

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart:205:
final Statement tryBlock;
On 2014/04/24 13:24:23, kmillikin wrote:
> The grammar has tryBlock, the body of CatchBlock, and finallyBlock as blocks,
> not statements.
> 
> There's an obvious interpretation here of any other statement as a singleton
> block --- but maybe it's worth calling it out in a comment?

Done.

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart:231:
class VariableDefinitions extends Statement {
On 2014/04/24 13:24:23, kmillikin wrote:
> The spec uses variable 'declaration', we should too.

Done.

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart:254:
class FunctionStatement extends Statement {
On 2014/04/24 13:24:23, kmillikin wrote:
> FunctionDeclaration?

Done.

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart:383:
/// by [:toString():]. These string are then concatenated and returned.
On 2014/04/24 13:24:23, kmillikin wrote:
> `toString()`, and likewise below.

Done.

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart:502:
Increment.postfix(Expression expression, String operator): this(expression,
On 2014/04/24 13:24:23, kmillikin wrote:
> Break just before the colon (:), indent four spaces.

Done.

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart:676: if
(definesVariable(inner))
On 2014/04/24 13:24:23, kmillikin wrote:
> Braces.  The rule is one can only omit the braces if the body is a single
> statement, on the same line as the if, and there is no else.

Done.

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart:706: //
  Would there be a significant speedup using a Visitor or a method
On 2014/04/24 13:24:23, kmillikin wrote:
> Not only a speedup, but I think it would be more readable :)

The hard part is passing the parameters. I'll look into this later.

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart:721:
else if (e is FunctionExpression) {
On 2014/04/24 13:24:23, kmillikin wrote:
> Else on the same line as the closing brace to the if:
> 
> } else if (...) {

Done.

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart:1267:
OpenStringChunk chunk = new OpenStringChunk(null, q, getQuoteCost(q));
On 2014/04/24 13:24:23, kmillikin wrote:
> The definition of getQuoteCost is a long way away.  Make it local to this
> function?

Done.

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart:1295:
for (int i=0; i<parts.length; i++) {
On 2014/04/24 13:24:23, kmillikin wrote:
> Spaces around operators = and <, here and below.

Done.

https://codereview.chromium.org/250523003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_printer.dart:1309:
best[j].end(i),
On 2014/04/24 13:24:23, kmillikin wrote:
> I prefer not to close over i here.  Lift the function out of the loop and pass
> in i as endIndex.

Done.

[asgerf, 2014-04-25 12:35:45]

Committed patchset #3 manually as r35421 (presubmit successful).