Support for map literals in cps js emitter

Support for map literals in cps js emitter

R=floitsch@google.com

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

[sigurdm, 2014-11-27 14:49:10]

Patchset #1 (id:1) has been deleted

[sigurdm, 2014-11-27 14:51:04]

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

[floitsch, 2014-11-27 15:57:27]

LGTM.

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

https://codereview.chromium.org/761983002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:242: List<js.ArrayElement>
entries = new List<js.ArrayElement>();
I prefer to allocate the right size and to use a for loop:

int length = node.entries.length;
entries = new List<..>(length);
for (int i = 0; i < length; i++) {
  LiteralMapEntry entry = node.entries[i];
  entries[2*i] = new js.ArrayElement(i, visitExpression(entry.key));
  entries[2*i + 1] = ...
}

https://codereview.chromium.org/761983002/diff/20001/pkg/compiler/lib/src/uni...
File pkg/compiler/lib/src/universe/universe.dart (left):

https://codereview.chromium.org/761983002/diff/20001/pkg/compiler/lib/src/uni...
pkg/compiler/lib/src/universe/universe.dart:502: *
I don't like this change.
We shouldn't compile arguments that aren't needed.

If you can't avoid the compilation of arguments in some places, just make the
compileArgument the identity-function.

[sigurdm, 2014-11-28 09:24:24]

PTAL

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

https://codereview.chromium.org/761983002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:242: List<js.ArrayElement>
entries = new List<js.ArrayElement>();
On 2014/11/27 15:57:27, floitsch wrote:
> I prefer to allocate the right size and to use a for loop:
> 
> int length = node.entries.length;
> entries = new List<..>(length);
> for (int i = 0; i < length; i++) {
>   LiteralMapEntry entry = node.entries[i];
>   entries[2*i] = new js.ArrayElement(i, visitExpression(entry.key));
>   entries[2*i + 1] = ...
> }

Done.

https://codereview.chromium.org/761983002/diff/20001/pkg/compiler/lib/src/uni...
File pkg/compiler/lib/src/universe/universe.dart (left):

https://codereview.chromium.org/761983002/diff/20001/pkg/compiler/lib/src/uni...
pkg/compiler/lib/src/universe/universe.dart:502: *
On 2014/11/27 15:57:27, floitsch wrote:
> I don't like this change.
> We shouldn't compile arguments that aren't needed.
> 
> If you can't avoid the compilation of arguments in some places, just make the
> compileArgument the identity-function.

They are all compiled anyways.

[floitsch, 2014-11-28 09:37:04]

LGTM.

https://codereview.chromium.org/761983002/diff/40001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/codegen/codegen.dart (right):

https://codereview.chromium.org/761983002/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:177: arguments,
nit: I prefer to align them with the first argument.

https://codereview.chromium.org/761983002/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:245: entries[2 * i] =
Nit...
js.Expression key = visitExpression(node.entries[i].key);
js.Experssion value = visitExpression(node.entries[i].value);
entries[2 * i] = new js.ArrayElement(2 * i, key);
entries[2 * i + 1] = new js.ArrayElement(2 * i + 1, value);

Is this better than just using the new js.ArrayInitializer.from(...) ?


In theory you could have:
Iterable<js.Expression> entries = node.entries.expand((XYZ entry) {
  return [visitExpression(entry.key), visitExpression(entry.value)];
}
List<js.Expression> args = <js.Expression>[new
jsArrayInitializer.from(entries)];

But that's probably more expensive. Up to you.

[sigurdm, 2014-11-28 11:57:15]

Patchset #3 (id:60001) has been deleted

[sigurdm, 2014-11-28 12:03:54]

https://codereview.chromium.org/761983002/diff/40001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/codegen/codegen.dart (right):

https://codereview.chromium.org/761983002/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:177: arguments,
On 2014/11/28 09:37:03, floitsch wrote:
> nit: I prefer to align them with the first argument.

Done.

https://codereview.chromium.org/761983002/diff/40001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:245: entries[2 * i] =
On 2014/11/28 09:37:03, floitsch wrote:
> Nit...
> js.Expression key = visitExpression(node.entries[i].key);
> js.Experssion value = visitExpression(node.entries[i].value);
> entries[2 * i] = new js.ArrayElement(2 * i, key);
> entries[2 * i + 1] = new js.ArrayElement(2 * i + 1, value);
> 
Done
> Is this better than just using the new js.ArrayInitializer.from(...) ?
> 
In a coming CL I will remove ArrayElement and ArrayInitializer.from
> 
> In theory you could have:
> Iterable<js.Expression> entries = node.entries.expand((XYZ entry) {
>   return [visitExpression(entry.key), visitExpression(entry.value)];
> }
> List<js.Expression> args = <js.Expression>[new
> jsArrayInitializer.from(entries)];
> 
> But that's probably more expensive. Up to you.

Yes, I considered it - but it would allocate a lot of length 2 arrays.

[sigurdm, 2014-11-28 12:07:45]

Committed patchset #3 (id:80001) manually as 42029 (presubmit successful).