Make `let` usable as an identifier in ES6 sloppy mode.

Make `let` usable as an identifier in ES6 sloppy mode.

All of our mjsunit suite now runs through with --harmony-scoping enabled, up to expected failures (tests checking syntax errors for const/function in strict mode).

R=marja@chromium.org, ulan@chromium.org
BUG=v8:2198
LOG=Y

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

[marja, 2014-07-09 14:24:46]

https://codereview.chromium.org/378303003/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/378303003/diff/1/src/parser.cc#newcode1168
src/parser.cc:1168: if (allow_harmony_scoping() && strict_mode() == STRICT) {
(See below; isn't allow_harmony_scoping() necessarily true here.)

https://codereview.chromium.org/378303003/diff/1/src/parser.cc#newcode1557
src/parser.cc:1557: if (allow_harmony_scoping() && strict_mode() == STRICT) {
Ditto

https://codereview.chromium.org/378303003/diff/1/src/parser.cc#newcode1672
src/parser.cc:1672: if (allow_harmony_scoping() && strict_mode() == STRICT) {
I don't think we need to check allow_harmony_scoping() here; if it's not
allowed, we don't get Token::LET. Also, below you don't check it either.

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

https://codereview.chromium.org/378303003/diff/1/src/preparser.cc#newcode175
src/preparser.cc:175: if (allow_harmony_scoping() && strict_mode() == STRICT) {
Ditto

https://codereview.chromium.org/378303003/diff/1/src/preparser.cc#newcode306
src/preparser.cc:306: if (allow_harmony_scoping() && strict_mode() == STRICT) {
Ditto

[rossberg, 2014-07-09 14:28:50]

https://codereview.chromium.org/378303003/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/378303003/diff/1/src/parser.cc#newcode1672
src/parser.cc:1672: if (allow_harmony_scoping() && strict_mode() == STRICT) {
On 2014/07/09 14:24:45, marja wrote:
> I don't think we need to check allow_harmony_scoping() here; if it's not
> allowed, we don't get Token::LET. Also, below you don't check it either.

Yes, this was on purpose -- I wanted to be able to grep for harmony-scoping
specific code paths easily. But I can remove that if you prefer.

[marja, 2014-07-09 14:43:11]

On 2014/07/09 14:28:50, rossberg wrote:
> https://codereview.chromium.org/378303003/diff/1/src/parser.cc
> File src/parser.cc (right):
> 
> https://codereview.chromium.org/378303003/diff/1/src/parser.cc#newcode1672
> src/parser.cc:1672: if (allow_harmony_scoping() && strict_mode() == STRICT) {
> On 2014/07/09 14:24:45, marja wrote:
> > I don't think we need to check allow_harmony_scoping() here; if it's not
> > allowed, we don't get Token::LET. Also, below you don't check it either.
> 
> Yes, this was on purpose -- I wanted to be able to grep for harmony-scoping
> specific code paths easily. But I can remove that if you prefer.

I don't have an opinion, but now it's inconsistent (some places check it and
some not).

[marja, 2014-07-09 14:43:28]

On 2014/07/09 14:43:11, marja wrote:
> On 2014/07/09 14:28:50, rossberg wrote:
> > https://codereview.chromium.org/378303003/diff/1/src/parser.cc
> > File src/parser.cc (right):
> > 
> > https://codereview.chromium.org/378303003/diff/1/src/parser.cc#newcode1672
> > src/parser.cc:1672: if (allow_harmony_scoping() && strict_mode() == STRICT)
{
> > On 2014/07/09 14:24:45, marja wrote:
> > > I don't think we need to check allow_harmony_scoping() here; if it's not
> > > allowed, we don't get Token::LET. Also, below you don't check it either.
> > 
> > Yes, this was on purpose -- I wanted to be able to grep for harmony-scoping
> > specific code paths easily. But I can remove that if you prefer.
> 
> I don't have an opinion, but now it's inconsistent (some places check it and
> some not).

Err, meant to say I don't have a *strong* opinion :)

[marja, 2014-07-09 17:26:05]

Actually, how about you add ASSERT(allow_harmony_scoping()) instead? That'll
have both good sides: 1) you can grep allow_harmony_scoping 2) you don't need to
check a thing which is always true (and the reader won't wonder what happens in
that code path if allow_harmony_scoping() is false).

[ulan, 2014-07-10 07:10:04]

lgtm

> Actually, how about you add ASSERT(allow_harmony_scoping()) instead?
+1

[rossberg, 2014-07-10 14:02:18]

On 2014/07/09 17:26:05, marja wrote:
> Actually, how about you add ASSERT(allow_harmony_scoping()) instead? That'll
> have both good sides: 1) you can grep allow_harmony_scoping 2) you don't need
to
> check a thing which is always true (and the reader won't wonder what happens
in
> that code path if allow_harmony_scoping() is false).

Done.

[marja, 2014-07-10 14:05:28]

LGTM modulo comments:

1) it would be good to add test-parsing tests which use let as an identifier (so
we can make sure that Parser and PreParser agree -- no other tests are testing
that)

2) see below

https://codereview.chromium.org/378303003/diff/20001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/378303003/diff/20001/src/parser.cc#newcode2082
src/parser.cc:2082: } else if (peek() == Token::LET && strict_mode() == STRICT)
{
Here you probably also want to ASSERT(allow_harmony_scoping()) too.

https://codereview.chromium.org/378303003/diff/20001/src/parser.cc#newcode3121
src/parser.cc:3121: } else if (peek() == Token::LET && strict_mode() == STRICT)
{
Ditto

[rossberg, 2014-07-10 14:06:47]

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

[rossberg, 2014-07-10 14:08:24]

On 2014/07/10 14:05:28, marja wrote:
> LGTM modulo comments:

Oops, already committed before your reply. Will do that in a follow-up CL.

> 1) it would be good to add test-parsing tests which use let as an identifier
(so
> we can make sure that Parser and PreParser agree -- no other tests are testing
> that)
> 
> 2) see below
> 
> https://codereview.chromium.org/378303003/diff/20001/src/parser.cc
> File src/parser.cc (right):
> 
> https://codereview.chromium.org/378303003/diff/20001/src/parser.cc#newcode2082
> src/parser.cc:2082: } else if (peek() == Token::LET && strict_mode() ==
STRICT)
> {
> Here you probably also want to ASSERT(allow_harmony_scoping()) too.
> 
> https://codereview.chromium.org/378303003/diff/20001/src/parser.cc#newcode3121
> src/parser.cc:3121: } else if (peek() == Token::LET && strict_mode() ==
STRICT)
> {
> Ditto