Remove js.ArrayElement.

Remove js.ArrayElement.
Instead introduce js.ArrayHole to mark empty spots in Arrays.

BUG=
R=floitsch@google.com

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

[sigurdm, 2014-11-28 12:21:40]

sigurdm@google.com changed reviewers:
+ floitsch@google.com

[floitsch, 2014-11-28 12:42:50]

LGTM.
some nits (and feel free to ignore the "growable: false").

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js/prin...
File pkg/compiler/lib/src/js/printer.dart (right):

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js/prin...
pkg/compiler/lib/src/js/printer.dart:785: (i == 0 && elements[i] is ArrayHole))
{
Why the check for "i == 0" ?

I think it should be:

for (int i = 0 ... ) {
  Expression element = elements[i];
  if (element is ArrayHole) {
    // Note that array holes must have a trailing "," even if they are
    // in last position. Otherwise `[,]` (having length 1) would become
    // equal to `[]` (the empty array).
    out(",");
    continue;
  }
  if (i != 0) spaceOut();
  visitNestedExpression(entry, ...);
  // We can skip the trailing "," for the last element (since it's not
  // an array hole).
  if (i != node.length - 1) out(",");
}

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js/prin...
pkg/compiler/lib/src/js/printer.dart:787: spaceOut();
This means that we now have "[, ]" instead of "[,]".

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js/prin...
pkg/compiler/lib/src/js/printer.dart:793: visitArrayHole(ArrayHole node) {
Should be unreachable again, if you use my code.

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js/temp...
File pkg/compiler/lib/src/js/template.dart (right):

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js/temp...
pkg/compiler/lib/src/js/template.dart:618: .toList();
could be "growable: false".

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js/temp...
pkg/compiler/lib/src/js/template.dart:620: List<Expression> elements =
elementMakers.map(
I prefer:

  List<Expression> elements = elementMakers
      .map((Instantiator instantiator) => instantiator(arguments))
      .toList(growable: false);

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js_back...
File pkg/compiler/lib/src/js_backend/constant_emitter.dart (right):

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/constant_emitter.dart:365: return
values.map(constantEmitter.reference).toList();
growable: false ?

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js_back...
File pkg/compiler/lib/src/js_backend/runtime_types.dart (right):

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/runtime_types.dart:509: List<jsAst.Expression>
elements = types.map(
move ".map" into next line, and then ".toList" in the next.
(see other comments).

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js_emit...
File pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart (right):

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart:111:
holders.map((e) => new js.VariableUse(e.name)).toList()))
growable: false ?
but probably not enough space...

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart:201: new
js.ArrayInitializer(unit.libraries.map(emitLibrary).toList());
ditto.
May create a new local:
List<Expression> entries = unit.libraries.map(emitLibrary).toList();
js.ArrayInitializer content = new js.ArrayInitializer(entries);

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart:229: return new
js.ArrayInitializer(fieldDescriptors.toList());
ditto.

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart:252: new
js.ArrayInitializer(staticDescriptors.toList());
ditto.

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart:254: new
js.ArrayInitializer(classDescriptors.toList());
ditto.

[sigurdm, 2014-11-28 14:52:12]

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js/prin...
File pkg/compiler/lib/src/js/printer.dart (right):

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js/prin...
pkg/compiler/lib/src/js/printer.dart:785: (i == 0 && elements[i] is ArrayHole))
{
On 2014/11/28 12:42:50, floitsch wrote:
> Why the check for "i == 0" ?
> 
> I think it should be:
> 
> for (int i = 0 ... ) {
>   Expression element = elements[i];
>   if (element is ArrayHole) {
>     // Note that array holes must have a trailing "," even if they are
>     // in last position. Otherwise `[,]` (having length 1) would become
>     // equal to `[]` (the empty array).
>     out(",");
>     continue;
>   }
>   if (i != 0) spaceOut();
>   visitNestedExpression(entry, ...);
>   // We can skip the trailing "," for the last element (since it's not
>   // an array hole).
>   if (i != node.length - 1) out(",");
> }

Yes, something was wrong in my logic. Thanks.
Adding a test for the array-hole case.

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js/prin...
pkg/compiler/lib/src/js/printer.dart:787: spaceOut();
On 2014/11/28 12:42:50, floitsch wrote:
> This means that we now have "[, ]" instead of "[,]".

True
Fixed

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js/prin...
pkg/compiler/lib/src/js/printer.dart:793: visitArrayHole(ArrayHole node) {
On 2014/11/28 12:42:50, floitsch wrote:
> Should be unreachable again, if you use my code.

Done.

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js/temp...
File pkg/compiler/lib/src/js/template.dart (right):

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js/temp...
pkg/compiler/lib/src/js/template.dart:618: .toList();
On 2014/11/28 12:42:50, floitsch wrote:
> could be "growable: false".

Done.

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js/temp...
pkg/compiler/lib/src/js/template.dart:620: List<Expression> elements =
elementMakers.map(
On 2014/11/28 12:42:50, floitsch wrote:
> I prefer:
> 
>   List<Expression> elements = elementMakers
>       .map((Instantiator instantiator) => instantiator(arguments))
>       .toList(growable: false);

Done.

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js_back...
File pkg/compiler/lib/src/js_backend/constant_emitter.dart (right):

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/constant_emitter.dart:365: return
values.map(constantEmitter.reference).toList();
On 2014/11/28 12:42:50, floitsch wrote:
> growable: false ?

Done.

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js_back...
File pkg/compiler/lib/src/js_backend/runtime_types.dart (right):

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/runtime_types.dart:509: List<jsAst.Expression>
elements = types.map(
On 2014/11/28 12:42:50, floitsch wrote:
> move ".map" into next line, and then ".toList" in the next.
> (see other comments).

Done.

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js_emit...
File pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart (right):

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart:111:
holders.map((e) => new js.VariableUse(e.name)).toList()))
On 2014/11/28 12:42:50, floitsch wrote:
> growable: false ?
> but probably not enough space...

Done.

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart:201: new
js.ArrayInitializer(unit.libraries.map(emitLibrary).toList());
On 2014/11/28 12:42:50, floitsch wrote:
> ditto.
> May create a new local:
> List<Expression> entries = unit.libraries.map(emitLibrary).toList();
> js.ArrayInitializer content = new js.ArrayInitializer(entries);

Done.

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart:229: return new
js.ArrayInitializer(fieldDescriptors.toList());
On 2014/11/28 12:42:50, floitsch wrote:
> ditto.

Done.

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart:252: new
js.ArrayInitializer(staticDescriptors.toList());
On 2014/11/28 12:42:50, floitsch wrote:
> ditto.

Done.

https://codereview.chromium.org/750323003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart:254: new
js.ArrayInitializer(classDescriptors.toList());
On 2014/11/28 12:42:50, floitsch wrote:
> ditto.

Done.

[sigurdm, 2014-12-01 09:09:21]

ptal - I added parsing of array holes.

[floitsch, 2014-12-01 13:03:29]

Still LGTM.

https://codereview.chromium.org/750323003/diff/20001/pkg/compiler/lib/src/js/...
File pkg/compiler/lib/src/js/template.dart (right):

https://codereview.chromium.org/750323003/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/template.dart:617: .map((Expression element) =>
visit(element))
.map(visit)

https://codereview.chromium.org/750323003/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/template.dart:620: List<Expression> elements =
elementMakers.map(
move the ".map(" into the next line (hoping that it still fits).

https://codereview.chromium.org/750323003/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/runtime_types.dart (right):

https://codereview.chromium.org/750323003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/runtime_types.dart:510: types.map((DartType
type) => getTypeRepresentation(type, onVariable))
nit.
I prefer to keep "types" in the line above, and start this line with ".map"

https://codereview.chromium.org/750323003/diff/20001/tests/compiler/dart2js/j...
File tests/compiler/dart2js/js_parser_test.dart (right):

https://codereview.chromium.org/750323003/diff/20001/tests/compiler/dart2js/j...
tests/compiler/dart2js/js_parser_test.dart:13: print("About to parse
$expression");
Remove debug prints.

[sigurdm, 2014-12-02 09:27:31]

https://codereview.chromium.org/750323003/diff/20001/pkg/compiler/lib/src/js/...
File pkg/compiler/lib/src/js/template.dart (right):

https://codereview.chromium.org/750323003/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/template.dart:617: .map((Expression element) =>
visit(element))
On 2014/12/01 13:03:29, floitsch wrote:
> .map(visit)

Done.

https://codereview.chromium.org/750323003/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/template.dart:620: List<Expression> elements =
elementMakers.map(
On 2014/12/01 13:03:29, floitsch wrote:
> move the ".map(" into the next line (hoping that it still fits).

Done.

https://codereview.chromium.org/750323003/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/runtime_types.dart (right):

https://codereview.chromium.org/750323003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/runtime_types.dart:510: types.map((DartType
type) => getTypeRepresentation(type, onVariable))
On 2014/12/01 13:03:29, floitsch wrote:
> nit.
> I prefer to keep "types" in the line above, and start this line with ".map"

Done.

https://codereview.chromium.org/750323003/diff/20001/tests/compiler/dart2js/j...
File tests/compiler/dart2js/js_parser_test.dart (right):

https://codereview.chromium.org/750323003/diff/20001/tests/compiler/dart2js/j...
tests/compiler/dart2js/js_parser_test.dart:13: print("About to parse
$expression");
On 2014/12/01 13:03:29, floitsch wrote:
> Remove debug prints.

Done.

[sigurdm, 2014-12-02 09:28:46]

Committed patchset #3 (id:40001) manually as 42060 (presubmit successful).