[cleanup] refactor ParsePropertyDefinition for clarity

[cleanup] refactor ParsePropertyDefinition for clarity

Some cleanup of ParsePropertyDefinition --- Replaces certain hacks with
more structured, clean code, and adds additional comments to aid in
comprehension of this tricky area of the ambiguous recursive descent
parser.

BUG=v8:3583
LOG=N
R=adamk, aperez, wingo, rossberg

Committed: https://crrev.com/15e7897bec11abfcbc7fb177c74a3b649862c4d9
Cr-Commit-Position: refs/heads/master@{#30775}

[caitp (gmail), 2015-09-16 16:19:27]

PTAL --- this was some cleanup code which I was asked to split apart from a
bugfix CL.

It simplifies the code significantly, and it's likely that an optimizing
compiler won't have any trouble producing correct code with it.

[caitp (gmail), 2015-09-16 16:19:58]

On 2015/09/16 16:19:27, caitp wrote:
> PTAL --- this was some cleanup code which I was asked to split apart from a
> bugfix CL.
> 
> It simplifies the code significantly, and it's likely that an optimizing
> compiler won't have any trouble producing correct code with it. --- Sorry, I
meant to say, it's likely that it won't suffer
from any noticeable parsing performance regressions.

[adamk, 2015-09-16 19:03:14]

Mostly looks good, just some style nits (on both the structure and the comments;
I love having the grammar inline, btw).

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

https://codereview.chromium.org/1348773004/diff/1/src/preparser.h#newcode2615
src/preparser.h:2615: if (peek() == Token::COLON) {
Please add

DCHECK(!is_static)

above this line.

https://codereview.chromium.org/1348773004/diff/1/src/preparser.h#newcode2617
src/preparser.h:2617: //     PropertyName : AssignmentExpression
Please quote the ':' in this production (otherwise it reads ambiguously)

https://codereview.chromium.org/1348773004/diff/1/src/preparser.h#newcode2627
src/preparser.h:2627: } else if (Token::IsIdentifier(name_token,
language_mode(),
Style nit: I think it would be easier to read this as a separate if statement,
given that you're returning from the 'if' branch.

https://codereview.chromium.org/1348773004/diff/1/src/preparser.h#newcode2636
src/preparser.h:2636: //     IdentifierReference : AssignmentExpression
Do you mean

"IdentifierReference Initializer"

?

https://codereview.chromium.org/1348773004/diff/1/src/preparser.h#newcode2668
src/preparser.h:2668: //     PropertyName ( StrictFormalParameters ) {
FunctionBody }
More quoting, please. Not going to mark up the rest of these, but all literal
tokens should be quoted.

https://codereview.chromium.org/1348773004/diff/1/src/preparser.h#newcode2696
src/preparser.h:2696: } else if (in_class && name_is_static && !is_static) {
Nit again: I'd recommend making this not an 'else' clause but a separate if
statement, given that the return above.

https://codereview.chromium.org/1348773004/diff/1/src/preparser.h#newcode2739
src/preparser.h:2739: } else {
You can move the contents of the else clause up to the function level...

https://codereview.chromium.org/1348773004/diff/1/src/preparser.h#newcode2746
src/preparser.h:2746: UNREACHABLE();
...and remove this line.

[caitp (gmail), 2015-09-16 20:10:26]

Comments addressed, thanks for the look

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

https://codereview.chromium.org/1348773004/diff/1/src/preparser.h#newcode2615
src/preparser.h:2615: if (peek() == Token::COLON) {
On 2015/09/16 19:03:14, adamk wrote:
> Please add
> 
> DCHECK(!is_static)
> 
> above this line.

Done.

https://codereview.chromium.org/1348773004/diff/1/src/preparser.h#newcode2617
src/preparser.h:2617: //     PropertyName : AssignmentExpression
On 2015/09/16 19:03:14, adamk wrote:
> Please quote the ':' in this production (otherwise it reads ambiguously)

Done.

https://codereview.chromium.org/1348773004/diff/1/src/preparser.h#newcode2627
src/preparser.h:2627: } else if (Token::IsIdentifier(name_token,
language_mode(),
On 2015/09/16 19:03:13, adamk wrote:
> Style nit: I think it would be easier to read this as a separate if statement,
> given that you're returning from the 'if' branch.

Done.

https://codereview.chromium.org/1348773004/diff/1/src/preparser.h#newcode2636
src/preparser.h:2636: //     IdentifierReference : AssignmentExpression
On 2015/09/16 19:03:14, adamk wrote:
> Do you mean
> 
> "IdentifierReference Initializer"
> 
> ?

Yes, not sure how this got so mixed up =) Done --- I dunno if you prefer `(opt)`
as in the draft, or a `?` as it usually shows up in yacc/bison, or what. I went
with (opt), but it looks weird without being in subscript.

https://codereview.chromium.org/1348773004/diff/1/src/preparser.h#newcode2668
src/preparser.h:2668: //     PropertyName ( StrictFormalParameters ) {
FunctionBody }
On 2015/09/16 19:03:14, adamk wrote:
> More quoting, please. Not going to mark up the rest of these, but all literal
> tokens should be quoted.

Done.

https://codereview.chromium.org/1348773004/diff/1/src/preparser.h#newcode2696
src/preparser.h:2696: } else if (in_class && name_is_static && !is_static) {
On 2015/09/16 19:03:14, adamk wrote:
> Nit again: I'd recommend making this not an 'else' clause but a separate if
> statement, given that the return above.

Done.

https://codereview.chromium.org/1348773004/diff/1/src/preparser.h#newcode2739
src/preparser.h:2739: } else {
On 2015/09/16 19:03:13, adamk wrote:
> You can move the contents of the else clause up to the function level...

Done.

https://codereview.chromium.org/1348773004/diff/1/src/preparser.h#newcode2746
src/preparser.h:2746: UNREACHABLE();
On 2015/09/16 19:03:13, adamk wrote:
> ...and remove this line.

Done.

https://codereview.chromium.org/1348773004/diff/20001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1348773004/diff/20001/src/preparser.h#newcode...
src/preparser.h:2712: //    set PropertyName '(' PropertySetParameterList ')'
'{' FunctionBody '}'
The get/set should probably be quoted, but there isn't a lot of room left on the
line to add another 2 characters :(

[adamk, 2015-09-16 20:58:01]

lgtm once comments addressed

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

https://codereview.chromium.org/1348773004/diff/1/src/preparser.h#newcode2636
src/preparser.h:2636: //     IdentifierReference : AssignmentExpression
On 2015/09/16 20:10:25, caitp wrote:
> On 2015/09/16 19:03:14, adamk wrote:
> > Do you mean
> > 
> > "IdentifierReference Initializer"
> > 
> > ?
> 
> Yes, not sure how this got so mixed up =) Done --- I dunno if you prefer
`(opt)`
> as in the draft, or a `?` as it usually shows up in yacc/bison, or what. I
went
> with (opt), but it looks weird without being in subscript.

I'd go with the ?, that's what we use elsewhere in this file.

https://codereview.chromium.org/1348773004/diff/20001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1348773004/diff/20001/src/preparser.h#newcode...
src/preparser.h:2704: //    static MethodDefinition
'static' should be quoted.

https://codereview.chromium.org/1348773004/diff/20001/src/preparser.h#newcode...
src/preparser.h:2712: //    set PropertyName '(' PropertySetParameterList ')'
'{' FunctionBody '}'
On 2015/09/16 20:10:25, caitp wrote:
> The get/set should probably be quoted, but there isn't a lot of room left on
the
> line to add another 2 characters :(

Yuck, 80 char limit is unfortunate here. Probably more readable all on one line
than quoted and split into two, so fine to leave it.

[caitp (gmail), 2015-09-16 21:01:09]

Nice to make it a bit easier to understand, CQing in a moment

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

https://codereview.chromium.org/1348773004/diff/1/src/preparser.h#newcode2636
src/preparser.h:2636: //     IdentifierReference : AssignmentExpression
On 2015/09/16 20:58:00, adamk wrote:
> On 2015/09/16 20:10:25, caitp wrote:
> > On 2015/09/16 19:03:14, adamk wrote:
> > > Do you mean
> > > 
> > > "IdentifierReference Initializer"
> > > 
> > > ?
> > 
> > Yes, not sure how this got so mixed up =) Done --- I dunno if you prefer
> `(opt)`
> > as in the draft, or a `?` as it usually shows up in yacc/bison, or what. I
> went
> > with (opt), but it looks weird without being in subscript.
> 
> I'd go with the ?, that's what we use elsewhere in this file.

Done.

https://codereview.chromium.org/1348773004/diff/20001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1348773004/diff/20001/src/preparser.h#newcode...
src/preparser.h:2704: //    static MethodDefinition
On 2015/09/16 20:58:00, adamk wrote:
> 'static' should be quoted.

Done.

https://codereview.chromium.org/1348773004/diff/20001/src/preparser.h#newcode...
src/preparser.h:2712: //    set PropertyName '(' PropertySetParameterList ')'
'{' FunctionBody '}'
On 2015/09/16 20:58:00, adamk wrote:
> On 2015/09/16 20:10:25, caitp wrote:
> > The get/set should probably be quoted, but there isn't a lot of room left on
> the
> > line to add another 2 characters :(
> 
> Yuck, 80 char limit is unfortunate here. Probably more readable all on one
line
> than quoted and split into two, so fine to leave it.

Acknowledged.

[caitp (gmail), 2015-09-16 21:01:27]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-09-16 21:01:27]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/1348773004/#ps40001
(title: "Few more comments fixed up")

[commit-bot: I haz the power, 2015-09-16 21:01:40]

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

[commit-bot: I haz the power, 2015-09-16 21:27:26]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-16 21:27:49]

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