Add support for local variables(including parameters) to cps->js.

Add support for local variables(including parameters) to cps->js.

R=floitsch@google.com

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

[sigurdm, 2014-11-13 14:08:14]

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

[floitsch, 2014-11-13 15:59:32]

FYI.

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

https://codereview.chromium.org/726543002/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:35: /// Variables that have
had their declaration created.
I'm not sure I understand that comment.

https://codereview.chromium.org/726543002/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:58:
variableNames[parameter] = parameter.element.name;
We will need to filter names here.
There are some valid Dart names, that can't be used in JS. (Ex: 'function').

language/naming2_test should test this.

Should we go through 'getVariableName'?

https://codereview.chromium.org/726543002/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:65: // if their first
assignment could be pulled into the initializer.
Not completely clear what this means.

https://codereview.chromium.org/726543002/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:294: // Try to pull into
initializer.
I don't think we want to do this.
We found that:
var x, y, z;
x = XYZ;
y = abc;
z = SSS;

is smaller than:
var x = XYZ;
var y = abc;
var z = SSS;

(assuming that we can't initialize the variables directly in the declaration
list: var x = XYZ, y = abc, z = SSS);

Also note that the JS tree already does the same thing again.
We might just want to make it possible to move the assignments into the list
(although "var a, b, c, d..." is extremely well gzipped).

https://codereview.chromium.org/726543002/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:296: // if
(isNullLiteral(definition)) definition = null;
commented code.

https://codereview.chromium.org/726543002/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:308: // if
(isNullLiteral(definition)) definition = null;
commented code.

https://codereview.chromium.org/726543002/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:315:
visitStatement(node.next);
I'm surprised that the visit-functions have to trigger the visit of the next
instruction. But I guess this is part of the IR visitor...

[sigurdm, 2014-11-18 15:17:46]

PTAL

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

https://codereview.chromium.org/726543002/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:35: /// Variables that have
had their declaration created.
On 2014/11/13 15:59:32, floitsch wrote:
> I'm not sure I understand that comment.

I tried to improve it.

https://codereview.chromium.org/726543002/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:58:
variableNames[parameter] = parameter.element.name;
On 2014/11/13 15:59:32, floitsch wrote:
> We will need to filter names here.
> There are some valid Dart names, that can't be used in JS. (Ex: 'function').
> 
> language/naming2_test should test this.
> 
> Should we go through 'getVariableName'?
Yes I think that is the right thing to do. I think this is not done for
dart2dart because we prefer to preserve the parameter names.

I added a call to the safe name functionality to getVariableName

https://codereview.chromium.org/726543002/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:294: // Try to pull into
initializer.
On 2014/11/13 15:59:32, floitsch wrote:
> I don't think we want to do this.
> We found that:
> var x, y, z;
> x = XYZ;
> y = abc;
> z = SSS;
> 
> is smaller than:
> var x = XYZ;
> var y = abc;
> var z = SSS;
> 
> (assuming that we can't initialize the variables directly in the declaration
> list: var x = XYZ, y = abc, z = SSS);
> 
> Also note that the JS tree already does the same thing again.
> We might just want to make it possible to move the assignments into the list
> (although "var a, b, c, d..." is extremely well gzipped).

Ok, removed for now.

https://codereview.chromium.org/726543002/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:296: // if
(isNullLiteral(definition)) definition = null;
On 2014/11/13 15:59:32, floitsch wrote:
> commented code.

Removed

https://codereview.chromium.org/726543002/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:308: // if
(isNullLiteral(definition)) definition = null;
On 2014/11/13 15:59:32, floitsch wrote:
> commented code.

Done.

https://codereview.chromium.org/726543002/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:315:
visitStatement(node.next);
On 2014/11/13 15:59:32, floitsch wrote:
> I'm surprised that the visit-functions have to trigger the visit of the next
> instruction. But I guess this is part of the IR visitor...

Yes - this is a side effect of how the tree IR is structured.

[floitsch, 2014-11-18 19:00:44]

LGTM.

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

https://codereview.chromium.org/726543002/diff/60001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:271: if
(node.variable.element is FunctionElement &&
I don't think we need to special case function-expressions.
The visitFunctionExpression will not be implemented anytime soon, and when we
do, we can look into how we want to do it.
Until then just remove the 'if', and let the visitFunctionExpression giveUp.

https://codereview.chromium.org/726543002/diff/60001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:285: // This is to ensure
that a fresh closure variable is created.
That doesn't make sense in the dart2js backend.

I would just remove this if too, and remove the declaredVariables list (and
"addDeclaredVariable" function) entirely.

[sigurdm, 2014-11-19 11:59:01]

Committed patchset #5 (id:80001) manually as 41827 (presubmit successful).

[floitsch, 2014-11-19 12:02:19]

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

https://codereview.chromium.org/726543002/diff/80001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:253: bool isFirstOccurrence
= (variableNames[node.variable] == null);
unused

https://codereview.chromium.org/726543002/diff/80001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:254: bool isDeclaredHere =
node.variable.host.element == currentFunction;
unused.

https://codereview.chromium.org/726543002/diff/80001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:255: String name =
getVariableName(node.variable);
unused.

[sigurdm, 2014-11-20 12:22:40]

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

https://codereview.chromium.org/726543002/diff/80001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:253: bool isFirstOccurrence
= (variableNames[node.variable] == null);
On 2014/11/19 12:02:19, floitsch wrote:
> unused

Fixed by https://codereview.chromium.org/737363002

https://codereview.chromium.org/726543002/diff/80001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:254: bool isDeclaredHere =
node.variable.host.element == currentFunction;
On 2014/11/19 12:02:19, floitsch wrote:
> unused.

Done.

https://codereview.chromium.org/726543002/diff/80001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:255: String name =
getVariableName(node.variable);
On 2014/11/19 12:02:19, floitsch wrote:
> unused.

Done.