cpsir: implementation of jsinterop in cps ir

cpsir: implementation of jsinterop in cps ir

R=jacobr@google.com

Committed: https://github.com/dart-lang/sdk/commit/a0c438a972bb4e8bc47881cbb948c3ed74aab00e

[Siggi Cherem (dart-lang), 2016-02-11 02:51:23]

Patchset #1 (id:1) has been deleted

[Siggi Cherem (dart-lang), 2016-02-11 02:58:53]

Patchset #1 (id:20001) has been deleted

[Siggi Cherem (dart-lang), 2016-02-11 03:12:21]

Patchset #1 (id:40001) has been deleted

[Siggi Cherem (dart-lang), 2016-02-11 03:13:30]

sigmund@google.com changed reviewers:
+ jacobr@google.com, kmillikin@google.com

[Siggi Cherem (dart-lang), 2016-02-11 03:13:31]

https://codereview.chromium.org/1688433006/diff/60001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart (right):

https://codereview.chromium.org/1688433006/diff/60001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2079: ir.Primitive
buildInvokeJsInteropMember(FunctionElement element,
most of this code logic is line-per-line as it was in SSA, there are a couple
minor differences that I'll point out below.

https://codereview.chromium.org/1688433006/diff/60001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2113: code = target;
difference 1: I use `target` directly here, SSA created a code template for it,
it seems I don't need to nest 2 templates for it.

https://codereview.chromium.org/1688433006/diff/60001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2120: return
buildForeignCode(js.js.parseForeignJS(code), inputs, behavior);
difference 2: I no longer attach the code to the behavior (behavior.codeTemplate
= ...). It seemed. unnecessary.

https://codereview.chromium.org/1688433006/diff/60001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2121: // TODO(sigmund): should
we record the source-information here?
difference 3: I have no place to store source-info here. Should we add it to
ForeignCode?

[Jacob, 2016-02-11 17:54:33]

lgtm with minor comments as far as js interop is concerned.

https://codereview.chromium.org/1688433006/diff/60001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart (right):

https://codereview.chromium.org/1688433006/diff/60001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:698: } else {
style not: remove else.

https://codereview.chromium.org/1688433006/diff/60001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2079: ir.Primitive
buildInvokeJsInteropMember(FunctionElement element,
On 2016/02/11 03:13:30, Siggi Cherem (dart-lang) wrote:
> most of this code logic is line-per-line as it was in SSA, there are a couple
> minor differences that I'll point out below.

Acknowledged.

https://codereview.chromium.org/1688433006/diff/60001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2084: // TODO(jacobr): assert
that the trailing arguments are all null.
hey now... shouldn't this be 
TODO(sigmund)? 
:)

https://codereview.chromium.org/1688433006/diff/60001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2113: code = target;
On 2016/02/11 03:13:30, Siggi Cherem (dart-lang) wrote:
> difference 1: I use `target` directly here, SSA created a code template for
it,
> it seems I don't need to nest 2 templates for it.

Acknowledged.

https://codereview.chromium.org/1688433006/diff/60001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2121: // TODO(sigmund): should
we record the source-information here?
On 2016/02/11 03:13:31, Siggi Cherem (dart-lang) wrote:
> difference 3: I have no place to store source-info here. Should we add it to
> ForeignCode?

Will that result in a good source maps exerience using the CPS ir?

https://codereview.chromium.org/1688433006/diff/60001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2644: } else {
sme nit about unneeded else

https://codereview.chromium.org/1688433006/diff/60001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart (right):

https://codereview.chromium.org/1688433006/diff/60001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:801: // on purpose (Issue
#25759).
Change comment to 
JS interop does not allow default values as JS does not support default values.
Lets trust that we have proper compile time errors for this.

https://codereview.chromium.org/1688433006/diff/60001/tests/html/js_typed_int...
File tests/html/js_typed_interop_test.dart (right):

https://codereview.chromium.org/1688433006/diff/60001/tests/html/js_typed_int...
tests/html/js_typed_interop_test.dart:129: external static num
multiplyDefault2(num a, [num b = 3]);
instead of adding this test here, add this as a language test as this should
test that a compile error should occur not that we generate plausible code,.

[Siggi Cherem (dart-lang), 2016-02-11 21:16:53]

thx

https://codereview.chromium.org/1688433006/diff/60001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart (right):

https://codereview.chromium.org/1688433006/diff/60001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:698: } else {
On 2016/02/11 17:54:33, Jacob wrote:
> style not: remove else.

Done.

https://codereview.chromium.org/1688433006/diff/60001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2084: // TODO(jacobr): assert
that the trailing arguments are all null.
On 2016/02/11 17:54:33, Jacob wrote:
> hey now... shouldn't this be 
> TODO(sigmund)? 
> :)

The comment was there before, I just made it a TODO :).. Added my name to the
mix :)

https://codereview.chromium.org/1688433006/diff/60001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2121: // TODO(sigmund): should
we record the source-information here?
On 2016/02/11 17:54:33, Jacob wrote:
> On 2016/02/11 03:13:31, Siggi Cherem (dart-lang) wrote:
> > difference 3: I have no place to store source-info here. Should we add it to
> > ForeignCode?
> 
> Will that result in a good source maps exerience using the CPS ir?

Yeah, I think that is likely to be an issue here and we need to add it. But
cps-ir source-maps plumbing is still missing, so we might need to wait until we
have it in place.

https://codereview.chromium.org/1688433006/diff/60001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2644: } else {
On 2016/02/11 17:54:33, Jacob wrote:
> sme nit about unneeded else

Done.

https://codereview.chromium.org/1688433006/diff/60001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart (right):

https://codereview.chromium.org/1688433006/diff/60001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:801: // on purpose (Issue
#25759).
On 2016/02/11 17:54:33, Jacob wrote:
> Change comment to 
> JS interop does not allow default values as JS does not support default
values.
> Lets trust that we have proper compile time errors for this.

Done.

https://codereview.chromium.org/1688433006/diff/60001/tests/html/js_typed_int...
File tests/html/js_typed_interop_test.dart (right):

https://codereview.chromium.org/1688433006/diff/60001/tests/html/js_typed_int...
tests/html/js_typed_interop_test.dart:129: external static num
multiplyDefault2(num a, [num b = 3]);
On 2016/02/11 17:54:33, Jacob wrote:
> instead of adding this test here, add this as a language test as this should
> test that a compile error should occur not that we generate plausible code,.

I got the test out on a separate file, however, I'm not sure whether it should
go in language or somewhere else. I kept it in html for now. Maybe all these
tests should move under pkg/js/test/ ?

[Siggi Cherem (dart-lang), 2016-02-11 21:21:56]

Description was changed from

==========
cpsir: implementation of jsinterop in cps ir
==========

to

==========
cpsir: implementation of jsinterop in cps ir

R=jacobr@google.com

Committed:
https://github.com/dart-lang/sdk/commit/a0c438a972bb4e8bc47881cbb948c3ed74aab00e
==========

[Siggi Cherem (dart-lang), 2016-02-11 21:21:57]

Committed patchset #2 (id:80001) manually as
a0c438a972bb4e8bc47881cbb948c3ed74aab00e (presubmit successful).