Make let variables fresh in each iteration of a for-loop.

Make let variables fresh in each iteration of a for-loop.

BUG=v8:2198
LOG=N
TEST=mjsunit/harmony/block-for
R=rossberg@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=21480

[ulan, 2014-05-22 10:00:26]

PTAL

[rossberg, 2014-05-22 12:55:44]

Readiing the CL I realised that our parsing of for-loops is not quite correct
yet: we do not yet allow arbitrary lexical declarations as init statements, like
the spec prescribes.

Also, I think the logic should ignore immutable variables in the init, since
they don't require copying.

But that's stuff for separate CLs.

https://codereview.chromium.org/292743009/diff/60001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/292743009/diff/60001/src/parser.cc#newcode2858
src/parser.cc:2858: // loop is executed to update the loop variables. The the
loop condition is
Nit: "The the"

https://codereview.chromium.org/292743009/diff/60001/src/parser.cc#newcode2886
src/parser.cc:2886: //  }
So, eventually, there should be a comment here along the lines of:

"Similarly for other mutable lexical declarations (e.g. function). Immutable
declarations (like const) do not need to be rewritten."

https://codereview.chromium.org/292743009/diff/60001/src/parser.cc#newcode2926
src/parser.cc:2926: Statement* body = ParseStatement(NULL, CHECK_OK);
It's somewhat inappropriate to do actual parsing in a function called Desugar. I
would pass this in as an argument.

https://codereview.chromium.org/292743009/diff/60001/src/parser.cc#newcode3008
src/parser.cc:3008: ZoneStringList let_bindings(1, zone());
lexical_bindings

https://codereview.chromium.org/292743009/diff/60001/src/parser.cc#newcode3164
src/parser.cc:3164: // If there are let bindings, then condition and the next
statement of the
"let bindings" -> "lexical declarations"

https://codereview.chromium.org/292743009/diff/60001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/292743009/diff/60001/src/parser.h#newcode721
src/parser.h:721: Statement* DesugarLetBindingsInForStatement(
Nit: DesugarLexicalDeclarationInForStatement, since in fact, it might also be
const (or others).

https://codereview.chromium.org/292743009/diff/60001/src/parser.h#newcode724
src/parser.h:724: Statement* Assign(Token::Value op, VariableProxy* dst,
Expression* src,
I am not sure if it's actually worth introducing these extra functions. But if
you do, they should probably be named NewAssignmentStatement and NewTemporary
for consistency with other such helpers. Also, please then update other places
in the parser calling NewAssignmentExpression and Scope::NewTemporary to use
them consistently.

[rossberg, 2014-05-22 13:13:43]

On 2014/05/22 12:55:44, rossberg wrote:
> Readiing the CL I realised that our parsing of for-loops is not quite correct
> yet: we do not yet allow arbitrary lexical declarations as init statements,
like
> the spec prescribes.

Sorry, strike that, the spec's LexicalDeclaration production does not include
'function'.

[ulan, 2014-05-23 14:29:03]

Thank you for review. I uploaded new patch set.

https://codereview.chromium.org/292743009/diff/60001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/292743009/diff/60001/src/parser.cc#newcode2858
src/parser.cc:2858: // loop is executed to update the loop variables. The the
loop condition is
On 2014/05/22 12:55:44, rossberg wrote:
> Nit: "The the"

Done.

https://codereview.chromium.org/292743009/diff/60001/src/parser.cc#newcode2886
src/parser.cc:2886: //  }
On 2014/05/22 12:55:44, rossberg wrote:
> So, eventually, there should be a comment here along the lines of:
> 
> "Similarly for other mutable lexical declarations (e.g. function). Immutable
> declarations (like const) do not need to be rewritten."

As discussed offline, only lets and consts are allowed here.

https://codereview.chromium.org/292743009/diff/60001/src/parser.cc#newcode2926
src/parser.cc:2926: Statement* body = ParseStatement(NULL, CHECK_OK);
On 2014/05/22 12:55:44, rossberg wrote:
> It's somewhat inappropriate to do actual parsing in a function called Desugar.
I
> would pass this in as an argument.

Done.

https://codereview.chromium.org/292743009/diff/60001/src/parser.cc#newcode3008
src/parser.cc:3008: ZoneStringList let_bindings(1, zone());
On 2014/05/22 12:55:44, rossberg wrote:
> lexical_bindings
Leaving as is.

https://codereview.chromium.org/292743009/diff/60001/src/parser.cc#newcode3164
src/parser.cc:3164: // If there are let bindings, then condition and the next
statement of the
On 2014/05/22 12:55:44, rossberg wrote:
> "let bindings" -> "lexical declarations"
Leaving as is.

https://codereview.chromium.org/292743009/diff/60001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/292743009/diff/60001/src/parser.h#newcode721
src/parser.h:721: Statement* DesugarLetBindingsInForStatement(
On 2014/05/22 12:55:44, rossberg wrote:
> Nit: DesugarLexicalDeclarationInForStatement, since in fact, it might also be
> const (or others).

As discussed offline, const and others are not handled here.

https://codereview.chromium.org/292743009/diff/60001/src/parser.h#newcode724
src/parser.h:724: Statement* Assign(Token::Value op, VariableProxy* dst,
Expression* src,
On 2014/05/22 12:55:44, rossberg wrote:
> I am not sure if it's actually worth introducing these extra functions. But if
> you do, they should probably be named NewAssignmentStatement and NewTemporary
> for consistency with other such helpers. Also, please then update other places
> in the parser calling NewAssignmentExpression and Scope::NewTemporary to use
> them consistently.

Done. Inlined the functions to be consistent.

[rossberg, 2014-05-23 15:21:42]

lgtm

[ulan, 2014-05-26 08:07:10]

Committed patchset #2 manually as r21480 (presubmit successful).