harmony-scoping: Allow 'const' iteration variables in strict mode.

harmony-scoping: Allow 'const' iteration variables in strict mode.

R=rossberg@chromium.org
BUG=v8:2506
LOG=N

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

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

[Dmitry Lomov (no reviews), 2014-10-22 14:57:30]

PTAL.

[rossberg, 2014-10-23 09:44:54]

https://codereview.chromium.org/671913002/diff/1/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/671913002/diff/1/src/preparser.cc#newcode466
src/preparser.cc:466: (is_strict_const && peek() == Token::COMMA)) {
I don't understand this condition. If it is true, you always will produce a
syntax error on the next line. Is that intentional? If so, perhaps put in an
explanatory comment (or refactor).

https://codereview.chromium.org/671913002/diff/1/src/preparser.cc#newcode684
src/preparser.cc:684: bool is_let_or_strict_const =
Nit: how about naming this 'is_lexical'?

https://codereview.chromium.org/671913002/diff/1/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/671913002/diff/1/test/cctest/test-parsing.cc#...
test/cctest/test-parsing.cc:4215: const char* data[] = {"for(const x = 1; ; )
{}", "for(const x in [1,2,3]) {}",
Add a case like "for (const x = 1, y = 2; ; )".

https://codereview.chromium.org/671913002/diff/1/test/cctest/test-parsing.cc#...
test/cctest/test-parsing.cc:4233: "for(const x = 4, y of [1,2,3]) {}",
Add "for (const x, y in/of [])".

https://codereview.chromium.org/671913002/diff/1/test/mjsunit/regress/regress...
File test/mjsunit/regress/regress-2506.js (right):

https://codereview.chromium.org/671913002/diff/1/test/mjsunit/regress/regress...
test/mjsunit/regress/regress-2506.js:6: 'use strict';
Nit: empty line before.

[Dmitry Lomov (no reviews), 2014-10-23 10:33:34]

Comments addressed.

https://codereview.chromium.org/671913002/diff/1/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/671913002/diff/1/src/preparser.cc#newcode466
src/preparser.cc:466: (is_strict_const && peek() == Token::COMMA)) {
On 2014/10/23 09:44:54, rossberg wrote:
> I don't understand this condition. If it is true, you always will produce a
> syntax error on the next line. Is that intentional? If so, perhaps put in an
> explanatory comment (or refactor).
Yes, intentional. Comment added.

https://codereview.chromium.org/671913002/diff/1/src/preparser.cc#newcode684
src/preparser.cc:684: bool is_let_or_strict_const =
On 2014/10/23 09:44:54, rossberg wrote:
> Nit: how about naming this 'is_lexical'?

Done.

https://codereview.chromium.org/671913002/diff/1/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/671913002/diff/1/test/cctest/test-parsing.cc#...
test/cctest/test-parsing.cc:4215: const char* data[] = {"for(const x = 1; ; )
{}", "for(const x in [1,2,3]) {}",
On 2014/10/23 09:44:54, rossberg wrote:
> Add a case like "for (const x = 1, y = 2; ; )".

Done.

https://codereview.chromium.org/671913002/diff/1/test/cctest/test-parsing.cc#...
test/cctest/test-parsing.cc:4233: "for(const x = 4, y of [1,2,3]) {}",
On 2014/10/23 09:44:54, rossberg wrote:
> Add "for (const x, y in/of [])".

Done.

https://codereview.chromium.org/671913002/diff/1/test/mjsunit/regress/regress...
File test/mjsunit/regress/regress-2506.js (right):

https://codereview.chromium.org/671913002/diff/1/test/mjsunit/regress/regress...
test/mjsunit/regress/regress-2506.js:6: 'use strict';
On 2014/10/23 09:44:54, rossberg wrote:
> Nit: empty line before.

Done.

[rossberg, 2014-10-23 10:41:28]

https://codereview.chromium.org/671913002/diff/20001/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/671913002/diff/20001/src/preparser.cc#newcode466
src/preparser.cc:466: // require initializers for multiple consts.
Nit: upper case

https://codereview.chromium.org/671913002/diff/20001/test/mjsunit/regress/reg...
File test/mjsunit/regress/regress-2506.js (right):

https://codereview.chromium.org/671913002/diff/20001/test/mjsunit/regress/reg...
test/mjsunit/regress/regress-2506.js:9: // Top-level code
Actually, on second though, we should have more tests. In particular, test that

- fresh bindings are correctly created for each iteration of the loop (using
closures),

- assignment is properly rejected (in strict and sloppy mode).

These are probably not regression tests, but should extend the respective tests
we already have for 'for-let-in/of'.

[rossberg, 2014-10-23 10:47:11]

lgtm

https://codereview.chromium.org/671913002/diff/20001/test/mjsunit/regress/reg...
File test/mjsunit/regress/regress-2506.js (right):

https://codereview.chromium.org/671913002/diff/20001/test/mjsunit/regress/reg...
test/mjsunit/regress/regress-2506.js:9: // Top-level code
On 2014/10/23 10:41:28, rossberg wrote:
> Actually, on second though, we should have more tests. In particular, test
that
> 
> - fresh bindings are correctly created for each iteration of the loop (using
> closures),
> 
> - assignment is properly rejected (in strict and sloppy mode).
> 
> These are probably not regression tests, but should extend the respective
tests
> we already have for 'for-let-in/of'.

Ah sorry, please ignore.

[Dmitry Lomov (no reviews), 2014-10-23 11:19:00]

Committed patchset #2 (id:20001) manually as 24834 (presubmit successful).

[Dmitry Lomov (no reviews), 2014-10-23 12:30:30]

Committed patchset #3 (id:40001) manually as 24842 (presubmit successful).