dart2js cps: Handle optional parameters in builder.

dart2js cps: Handle optional parameters in builder.

For static invocations, default values are inlined.

For all invocations, named parameters are reordered alphabetically.

BUG=
R=karlklose@google.com

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

Reverted: https://code.google.com/p/dart/source/detail?r=43679

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

[asgerf, 2015-02-09 14:44:29]

asgerf@google.com changed reviewers:
+ karlklose@google.com

[asgerf, 2015-02-09 14:44:30]

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

https://codereview.chromium.org/908863003/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:218: }
Arguments with side effects would get reordered here, e.g:

foo(b: getB(), a: getA());
-->
foo(getA(), getB());

[asgerf, 2015-02-09 14:48:03]

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

https://codereview.chromium.org/908863003/diff/20001/tests/compiler/dart2js/j...
tests/compiler/dart2js/js_backend_cps_ir_basic_test.dart:64: V.bar(v0, "b", v1);
This is a problem with the tree rewriter. It won't inline v1 because it thinks 7
and "b" might have side effects.

I'll fix this in a separate CL, probably by letting the rewriter understand that
constants are safe to propagate across other expressions.

[karlklose, 2015-02-10 09:07:45]

LGTM with comments.

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

https://codereview.chromium.org/908863003/diff/20001/pkg/compiler/lib/src/uni...
pkg/compiler/lib/src/universe/universe.dart:598: List
makeDynamicArgumentsList(List compiledArguments) {
I am not happy with having this helper here. Could we move it to the builder (or
is it used elsewhere?).

Please also add type arguments to the list annotation and constant.

https://codereview.chromium.org/908863003/diff/20001/pkg/compiler/lib/src/uni...
pkg/compiler/lib/src/universe/universe.dart:601: for (int i=0;
i<positionalArgumentCount; i++) {
Space around binary operators.

[asgerf, 2015-02-10 09:20:36]

New patchsets have been uploaded after l-g-t-m from karlklose@google.com

[asgerf, 2015-02-10 09:37:18]

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

https://codereview.chromium.org/908863003/diff/20001/pkg/compiler/lib/src/uni...
pkg/compiler/lib/src/universe/universe.dart:598: List
makeDynamicArgumentsList(List compiledArguments) {
On 2015/02/10 09:07:45, karlklose wrote:
> I am not happy with having this helper here. Could we move it to the builder
(or
> is it used elsewhere?).
> 
> Please also add type arguments to the list annotation and constant.

I agree.

I put it here because makeArgumentsList and makeArgumentsList2 are also here,
and it seemed inconsistent to have one and not the other in here. (Expand the
above 90 lines to see them.)

As with makeArgumentsList, we can't properly annotate List because the method is
generic. I didn't like the comment-style annotations used above.

It turns out one version of makeArgumentsList is only used in the CPS builder,
while the other is used from a couple of different places all tied to the SSA
builder. 

I've moved makeArgumentsList and makeDynamicArgumentsList into the CPS builder
and renamed makeArgumentsList2 -> makeArgumentsList for use by the SSA builder.

https://codereview.chromium.org/908863003/diff/20001/pkg/compiler/lib/src/uni...
pkg/compiler/lib/src/universe/universe.dart:601: for (int i=0;
i<positionalArgumentCount; i++) {
On 2015/02/10 09:07:45, karlklose wrote:
> Space around binary operators.

Done.

[asgerf, 2015-02-10 13:37:26]

Committed patchset #6 (id:100001) manually as 43643 (presubmit successful).

[asgerf, 2015-02-11 10:31:32]

PTAL at patch #8.

This was reverted because of a checked-mode error in dart2dart.

https://codereview.chromium.org/908863003/diff/120001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart (right):

https://codereview.chromium.org/908863003/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:735: arguments =
normalizeStaticArguments(selector, element, arguments);
This would fail in checked-mode when the element was not a FunctionElement.

[karlklose, 2015-02-11 14:54:44]

LGTM.

https://codereview.chromium.org/908863003/diff/140001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart (right):

https://codereview.chromium.org/908863003/diff/140001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:732: if (selector.isCall
&& (element.isGetter || element.isField)) {
Could you add a few comments for the different cases, perhaps with examples.

[asgerf, 2015-02-11 15:04:20]

New patchsets have been uploaded after l-g-t-m from karlklose@google.com

[asgerf, 2015-02-11 15:04:31]

https://codereview.chromium.org/908863003/diff/140001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart (right):

https://codereview.chromium.org/908863003/diff/140001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:732: if (selector.isCall
&& (element.isGetter || element.isField)) {
On 2015/02/11 14:54:43, karlklose wrote:
> Could you add a few comments for the different cases, perhaps with examples.

Done.

[asgerf, 2015-02-11 15:13:55]

Committed patchset #10 (id:130009) manually as 43684 (presubmit successful).

[karlklose, 2015-02-11 15:13:59]

SLGTM, thanks!