Inlining of calls with optional parameters.

Inlining of calls with optional parameters.


R=kmillikin@google.com

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

[zerny-google, 2012-10-23 10:30:03]

Rebased CL to current head.

[Kevin Millikin (Google), 2012-10-23 11:34:42]

LGTM with comments addressed.

https://codereview.chromium.org/11028140/diff/3001/runtime/vm/flow_graph_inli...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/11028140/diff/3001/runtime/vm/flow_graph_inli...
runtime/vm/flow_graph_inliner.cc:124: struct NamedArgument : ZoneAllocated {
It should work to make this ValueObject.

https://codereview.chromium.org/11028140/diff/3001/runtime/vm/flow_graph_inli...
runtime/vm/flow_graph_inliner.cc:603: for (intptr_t i = arg_count -
fixed_param_count;
I think it's a bit weird to adjust the initial value and limit like this.  It
seems more logical (follows on from the previous loop) if it is:

for (intptr_t i = arg_count; i < param_count; ++i) {
  ...values().At(i - fixed_param_count);
  ...
}

https://codereview.chromium.org/11028140/diff/3001/runtime/vm/flow_graph_inli...
runtime/vm/flow_graph_inliner.cc:646: for (intptr_t i = fixed_param_count; i <
param_count; i++) {
There's a mix of ++i and i++ in loops in this function.  Stick with one or the
other.

[zerny-google, 2012-10-23 13:03:41]

Thanks for the review. Uploading shortly.

https://codereview.chromium.org/11028140/diff/3001/runtime/vm/flow_graph_inli...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/11028140/diff/3001/runtime/vm/flow_graph_inli...
runtime/vm/flow_graph_inliner.cc:124: struct NamedArgument : ZoneAllocated {
On 2012/10/23 11:34:42, kmillikin wrote:
> It should work to make this ValueObject.

Done.

https://codereview.chromium.org/11028140/diff/3001/runtime/vm/flow_graph_inli...
runtime/vm/flow_graph_inliner.cc:603: for (intptr_t i = arg_count -
fixed_param_count;
On 2012/10/23 11:34:42, kmillikin wrote:
> I think it's a bit weird to adjust the initial value and limit like this.  It
> seems more logical (follows on from the previous loop) if it is:
> 
> for (intptr_t i = arg_count; i < param_count; ++i) {
>   ...values().At(i - fixed_param_count);
>   ...
> }

Done.

https://codereview.chromium.org/11028140/diff/3001/runtime/vm/flow_graph_inli...
runtime/vm/flow_graph_inliner.cc:646: for (intptr_t i = fixed_param_count; i <
param_count; i++) {
On 2012/10/23 11:34:42, kmillikin wrote:
> There's a mix of ++i and i++ in loops in this function.  Stick with one or the
> other.

Done.

[srdjan, 2012-10-23 16:00:52]

DBC

https://codereview.chromium.org/11028140/diff/3001/runtime/vm/flow_graph_inli...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/11028140/diff/3001/runtime/vm/flow_graph_inli...
runtime/vm/flow_graph_inliner.cc:34: DECLARE_FLAG(bool,
reject_named_argument_as_positional);
Remove?

[srdjan, 2012-10-23 16:02:36]

https://codereview.chromium.org/11028140/diff/3001/runtime/vm/flow_graph_inli...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/11028140/diff/3001/runtime/vm/flow_graph_inli...
runtime/vm/flow_graph_inliner.cc:34: DECLARE_FLAG(bool,
reject_named_argument_as_positional);
On 2012/10/23 16:00:52, srdjan wrote:
> Remove?

Actually not yet :-)... sorry