[parser] parenthesized Literals are not valid AssignmentPatterns

[parser] parenthesized Literals are not valid AssignmentPatterns

Encode "parenthesized" status of parenthesized Expressions to prevent
them from being treated as Patterns.

BUG=v8:4657, v8:811
LOG=N
R=rossberg@chromium.org, adamk@chromium.org, littledan@chromium.org

Committed: https://crrev.com/1f1af42d3a1d5282d333394bc2badb02e9fae2c6
Cr-Commit-Position: refs/heads/master@{#33190}

[caitp (gmail), 2016-01-08 15:24:35]

PTAL... parser fixup to avoid DCHECK

[caitp (gmail), 2016-01-08 15:24:48]

Description was changed from

==========
[es6] parenthesized Literals are not valid AssignmentPatterns

Encode "parenthesized" status of parenthesized Expressions to prevent
them from being treated as Patterns.

BUG=v8:4657, v8:811
LOG=N
R=rossberg@chromium.org, adamk@chromium.org, littledan@chromium.org
==========

to

==========
[parser] parenthesized Literals are not valid AssignmentPatterns

Encode "parenthesized" status of parenthesized Expressions to prevent
them from being treated as Patterns.

BUG=v8:4657, v8:811
LOG=N
R=rossberg@chromium.org, adamk@chromium.org, littledan@chromium.org
==========

[adamk, 2016-01-08 16:24:24]

Sad this isn't covered by test262, hopefully soon! Thanks for the quick fix.

https://codereview.chromium.org/1570793002/diff/1/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/1570793002/diff/1/src/ast/ast.h#newcode370
src/ast/ast.h:370: bit_field_ |= ParenthesizedField::encode(true);
Please use ParenthesizedField::update() for consistency

https://codereview.chromium.org/1570793002/diff/1/src/parsing/preparser.h
File src/parsing/preparser.h (right):

https://codereview.chromium.org/1570793002/diff/1/src/parsing/preparser.h#new...
src/parsing/preparser.h:314: // of the Type field, so they can share the
storage.
This comment is weird and out of date (some of the things below share storage,
and some do not. I'm guessing that's why you chose 31? Given that we appear to
have 32 bits available, could we just use the space we have available and wait
till later to do such micro-optimizations?

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

https://codereview.chromium.org/1570793002/diff/1/test/cctest/test-parsing.cc...
test/cctest/test-parsing.cc:7046: // v8:4657
Please add a few tests with multiple levels of parens?

[caitp (gmail), 2016-01-08 16:36:01]

thanks, comments (mostly) addressed

https://codereview.chromium.org/1570793002/diff/1/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/1570793002/diff/1/src/ast/ast.h#newcode370
src/ast/ast.h:370: bit_field_ |= ParenthesizedField::encode(true);
On 2016/01/08 16:24:23, adamk wrote:
> Please use ParenthesizedField::update() for consistency

all of the other uses seem to use ::encode() when the actual value is not a
variable (I don't think it should be variable here, fwiw)

--- done, but I protest :p

https://codereview.chromium.org/1570793002/diff/1/src/parsing/preparser.h
File src/parsing/preparser.h (right):

https://codereview.chromium.org/1570793002/diff/1/src/parsing/preparser.h#new...
src/parsing/preparser.h:314: // of the Type field, so they can share the
storage.
On 2016/01/08 16:24:23, adamk wrote:
> This comment is weird and out of date (some of the things below share storage,
> and some do not. I'm guessing that's why you chose 31? Given that we appear to
> have 32 bits available, could we just use the space we have available and wait
> till later to do such micro-optimizations?

Are you saying "We don't need to have both ExpressionType and Type? I think the
idea was to be able to classify expressions vs statements, without having a
"is_expression" bit somewhere. In any case, rearranging this seems outside of
the scope of this CL, so I assume this is just a general comment on how this
should change during parser cleanup

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

https://codereview.chromium.org/1570793002/diff/1/test/cctest/test-parsing.cc...
test/cctest/test-parsing.cc:7046: // v8:4657
On 2016/01/08 16:24:23, adamk wrote:
> Please add a few tests with multiple levels of parens?

Done

[adamk, 2016-01-08 16:52:53]

https://codereview.chromium.org/1570793002/diff/1/src/parsing/preparser.h
File src/parsing/preparser.h (right):

https://codereview.chromium.org/1570793002/diff/1/src/parsing/preparser.h#new...
src/parsing/preparser.h:314: // of the Type field, so they can share the
storage.
On 2016/01/08 16:36:01, caitp wrote:
> On 2016/01/08 16:24:23, adamk wrote:
> > This comment is weird and out of date (some of the things below share
storage,
> > and some do not. I'm guessing that's why you chose 31? Given that we appear
to
> > have 32 bits available, could we just use the space we have available and
wait
> > till later to do such micro-optimizations?
> 
> Are you saying "We don't need to have both ExpressionType and Type? I think
the
> idea was to be able to classify expressions vs statements, without having a
> "is_expression" bit somewhere. In any case, rearranging this seems outside of
> the scope of this CL, so I assume this is just a general comment on how this
> should change during parser cleanup

No, it's not a general comment, since you've seeming-randomly chosen the last
bit of the bitfield to store ParenthesizedField (which is independent of the
TypeField, and thus makes this comment untrue, since that line _is_ below the
comment).

I guess you could move the new line up above the comment, with a TODO to fix up
the mess in a followup.

[caitp (gmail), 2016-01-08 16:58:37]

https://codereview.chromium.org/1570793002/diff/1/src/parsing/preparser.h
File src/parsing/preparser.h (right):

https://codereview.chromium.org/1570793002/diff/1/src/parsing/preparser.h#new...
src/parsing/preparser.h:314: // of the Type field, so they can share the
storage.
On 2016/01/08 16:52:53, adamk wrote:
> On 2016/01/08 16:36:01, caitp wrote:
> > On 2016/01/08 16:24:23, adamk wrote:
> > > This comment is weird and out of date (some of the things below share
> storage,
> > > and some do not. I'm guessing that's why you chose 31? Given that we
appear
> to
> > > have 32 bits available, could we just use the space we have available and
> wait
> > > till later to do such micro-optimizations?
> > 
> > Are you saying "We don't need to have both ExpressionType and Type? I think
> the
> > idea was to be able to classify expressions vs statements, without having a
> > "is_expression" bit somewhere. In any case, rearranging this seems outside
of
> > the scope of this CL, so I assume this is just a general comment on how this
> > should change during parser cleanup
> 
> No, it's not a general comment, since you've seeming-randomly chosen the last
> bit of the bitfield to store ParenthesizedField (which is independent of the
> TypeField, and thus makes this comment untrue, since that line _is_ below the
> comment).
> 
> I guess you could move the new line up above the comment, with a TODO to fix
up
> the mess in a followup.

Alright, done

[adamk, 2016-01-08 17:19:40]

lgtm

[caitp (gmail), 2016-01-08 17:20:38]

The CQ bit was checked by caitpotter88@gmail.com

[commit-bot: I haz the power, 2016-01-08 17:20:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1570793002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1570793002/40001

[commit-bot: I haz the power, 2016-01-08 17:46:44]

Description was changed from

==========
[parser] parenthesized Literals are not valid AssignmentPatterns

Encode "parenthesized" status of parenthesized Expressions to prevent
them from being treated as Patterns.

BUG=v8:4657, v8:811
LOG=N
R=rossberg@chromium.org, adamk@chromium.org, littledan@chromium.org
==========

to

==========
[parser] parenthesized Literals are not valid AssignmentPatterns

Encode "parenthesized" status of parenthesized Expressions to prevent
them from being treated as Patterns.

BUG=v8:4657, v8:811
LOG=N
R=rossberg@chromium.org, adamk@chromium.org, littledan@chromium.org
==========

[commit-bot: I haz the power, 2016-01-08 17:46:44]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-08 17:47:25]

Description was changed from

==========
[parser] parenthesized Literals are not valid AssignmentPatterns

Encode "parenthesized" status of parenthesized Expressions to prevent
them from being treated as Patterns.

BUG=v8:4657, v8:811
LOG=N
R=rossberg@chromium.org, adamk@chromium.org, littledan@chromium.org
==========

to

==========
[parser] parenthesized Literals are not valid AssignmentPatterns

Encode "parenthesized" status of parenthesized Expressions to prevent
them from being treated as Patterns.

BUG=v8:4657, v8:811
LOG=N
R=rossberg@chromium.org, adamk@chromium.org, littledan@chromium.org

Committed: https://crrev.com/1f1af42d3a1d5282d333394bc2badb02e9fae2c6
Cr-Commit-Position: refs/heads/master@{#33190}
==========

[commit-bot: I haz the power, 2016-01-08 17:47:26]

Patchset 3 (id:??) landed as
https://crrev.com/1f1af42d3a1d5282d333394bc2badb02e9fae2c6
Cr-Commit-Position: refs/heads/master@{#33190}