[parser] Refactor of ParseClass* and ParseNativeDeclaration

[parser] Refactor of ParseClass* and ParseNativeDeclaration

This patch moves the following parsing method to ParserBase:

- ParseClassDeclaration
- ParseClassLiteral
- ParseNativeDeclaration

R=adamk@chromium.org, marja@chromium.org
BUG=
LOG=N

Committed: https://crrev.com/7818355363b7a66ff7709e33c72bfdef5eb21450
Committed: https://crrev.com/da33b67ad796364bf7c990667e32666c0ab1dc48
Cr-Original-Commit-Position: refs/heads/master@{#39814}
Cr-Commit-Position: refs/heads/master@{#39829}

[nickie, 2016-09-25 18:06:05]

The CQ bit was checked by nikolaos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-25 18:06:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-25 18:48:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-25 18:48:28]

Dry run: This issue passed the CQ dry run.

[marja, 2016-09-26 09:35:27]

https://codereview.chromium.org/2368083002/diff/20001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2368083002/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:686: : proxy(nullptr),
The creation & modification (set_end_pos) & usage of proxy are now somehow far
apart and difficult to follow... I don't have immediate ideas on how to make it
simpler though...

https://codereview.chromium.org/2368083002/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:4113: bool name_is_strict_reserved, int pos, bool* ok)
{
Can you rename pos into something more illustrative? It seems to be
class_token_position on the caller's side.

Can the positions be reordered; we're passing both class_name_location and
class_token_position, could we pass them in the opposite order so that the
smaller one is first?

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

https://codereview.chromium.org/2368083002/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:1599:
decl->proxy()->var()->set_initializer_position(position());
It's pretty... involved? fragile? that we pass "int pos" but still use
position() here. Are they different? Do they need to be? If yes, should we pass
in two positions (with descriptive names)? I guess pos <= position() here since
we have potentially advanced, but... is pos < position? Why does that make
sense?

https://codereview.chromium.org/2368083002/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:3591: : ast_value_factory()->empty_string());
Could we return here to reduce the indentation of the code currently in the else
branch?

https://codereview.chromium.org/2368083002/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:3632: if (!*ok) return;
Here you removed a DCHECK (which was incorrect, was DCHECK(ok) but most probably
wanted to be DCHECK(*ok)), why?

https://codereview.chromium.org/2368083002/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:3670:
class_info->proxy->var()->set_initializer_position(end_pos);
This code is now somehow... far away (logically) from creation of
class_info->proxy.

https://codereview.chromium.org/2368083002/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:3682: do_block->statements()->Add(
This function is (still) hard to follow without knowing the implementation
beforehand... since you know understand this better than I do, could you add
comments? :)

[nickie, 2016-09-26 12:19:59]

I addressed the relatively easy ones.  I will see if I can think of a better way
to refactor this, so that things that logically belong together are not
separated.

https://codereview.chromium.org/2368083002/diff/20001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2368083002/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:4113: bool name_is_strict_reserved, int pos, bool* ok)
{
On 2016/09/26 09:35:26, marja wrote:
> Can you rename pos into something more illustrative? It seems to be
> class_token_position on the caller's side.
> 
> Can the positions be reordered; we're passing both class_name_location and
> class_token_position, could we pass them in the opposite order so that the
> smaller one is first?

The 'pos' is the position of the class token, not the class name (which need not
exist).  I'm renaming it to make this clear.

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

https://codereview.chromium.org/2368083002/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:1599:
decl->proxy()->var()->set_initializer_position(position());
On 2016/09/26 09:35:27, marja wrote:
> It's pretty... involved? fragile? that we pass "int pos" but still use
> position() here. Are they different? Do they need to be? If yes, should we
pass
> in two positions (with descriptive names)? I guess pos <= position() here
since
> we have potentially advanced, but... is pos < position? Why does that make
> sense?

Again, 'pos' is the position of the class token, whereas position() is the
position of the initializer.  I'm renaming the former, to make this clear.

https://codereview.chromium.org/2368083002/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:3591: : ast_value_factory()->empty_string());
On 2016/09/26 09:35:27, marja wrote:
> Could we return here to reduce the indentation of the code currently in the
else
> branch?

Done.

https://codereview.chromium.org/2368083002/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:3632: if (!*ok) return;
On 2016/09/26 09:35:27, marja wrote:
> Here you removed a DCHECK (which was incorrect, was DCHECK(ok) but most
probably
> wanted to be DCHECK(*ok)), why?

I was not sure if this was meant to be DCHECK(*ok).
Adding it does not break any of the tests, so I'll do that.

[nickie, 2016-09-26 17:28:13]

https://codereview.chromium.org/2368083002/diff/20001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2368083002/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:686: : proxy(nullptr),
On 2016/09/26 09:35:26, marja wrote:
> The creation & modification (set_end_pos) & usage of proxy are now somehow far
> apart and difficult to follow... I don't have immediate ideas on how to make
it
> simpler though...

I'm afraid not much can be done about 'proxy' here.

In principle, the ClassInfo type (and all the *Info classes that I introduced in
the refactoring) should be something different for every parser implementation. 
The object's construction does some implementation-specific initialization
(therefore 'proxy', which is not even visible in parser base, is initialized if
needed).  It is then modified and/or consumed by other implementation methods. 
I think this design is quite reasonable --- only this class should be moved to
the parser.  The preparser could have fewer fields or be entirely dummy.

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

https://codereview.chromium.org/2368083002/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:3670:
class_info->proxy->var()->set_initializer_position(end_pos);
On 2016/09/26 09:35:27, marja wrote:
> This code is now somehow... far away (logically) from creation of
> class_info->proxy.

I covered this in my previous comment.  The field 'proxy' is assigned to in
DeclareClassVariable and used again here.  I'm afraid I can't see an alternative
design that works better, without exposing the proxy to the parser base.

https://codereview.chromium.org/2368083002/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:3682: do_block->statements()->Add(
On 2016/09/26 09:35:27, marja wrote:
> This function is (still) hard to follow without knowing the implementation
> beforehand... since you know understand this better than I do, could you add
> comments? :)

OK, here's what I can do.  I'm adding comments in all functions that take
ClassInfo as a parameter, explaining which fields they are using
(setting/getting).

[nickie, 2016-09-26 18:43:44]

The CQ bit was checked by nikolaos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 18:43:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 19:16:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-26 19:16:06]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-09-26 20:36:32]

Mostly lgtm save a few nits

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

https://codereview.chromium.org/2368083002/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:3575: // TODO(verwaest): declare via block_state.
Not sure this TODO is still relevant.

https://codereview.chromium.org/2368083002/diff/60001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2368083002/diff/60001/src/parsing/parser-base...
src/parsing/parser-base.h:4163: impl()->Infer();
This name should be longer, "Infer" is far too short to make it clear what this
is doing. Maybe InferFunctionName() should be renamed to
AddFunctionForNameInference() and then Infer() can be InferFunctionName()?

[marja, 2016-09-27 06:49:21]

lgtm w/ comment

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

https://codereview.chromium.org/2368083002/diff/60001/src/parsing/parser.cc#n...
src/parsing/parser.cc:1600:
decl->proxy()->var()->set_initializer_position(position());
IMO none of the Parser impl funcs should call position(), but they should get
passed all the data they need as parameters. This also makes it less fragile wrt
when exactly the impl func is called. Wdyt?

[nickie, 2016-09-27 08:52:17]

I'm cc:ing Dan, because of bakkot's CLs regarding class fields (2316233004,
2330473002) that he mentioned in an e-mail.  As this CL looks pretty much good,
by now, I don't think there's a reason not to land it.  Unless there are
objections, I'll do it later today.

If any of the class desugaring in bakkot's already landed CLs needs to be
reverted, it should be straightforward.

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

https://codereview.chromium.org/2368083002/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:3575: // TODO(verwaest): declare via block_state.
On 2016/09/26 20:36:32, adamk wrote:
> Not sure this TODO is still relevant.

Yes, I suppose not.  I'm removing it.

As a more general TODO, there's an inconsistency now in several places.  Toon
has tried to use "*State" objects that delegate things to scopes, instead of
using scope operations directly.  This is not in general followed in impl
methods, where I have avoided passing "*State" object pointers as parameters. 
Perhaps we should revisit this consistently in a subsequent cleanup.

https://codereview.chromium.org/2368083002/diff/60001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2368083002/diff/60001/src/parsing/parser-base...
src/parsing/parser-base.h:4163: impl()->Infer();
On 2016/09/26 20:36:32, adamk wrote:
> This name should be longer, "Infer" is far too short to make it clear what
this
> is doing. Maybe InferFunctionName() should be renamed to
> AddFunctionForNameInference() and then Infer() can be InferFunctionName()?

Done.

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

https://codereview.chromium.org/2368083002/diff/60001/src/parsing/parser.cc#n...
src/parsing/parser.cc:1600:
decl->proxy()->var()->set_initializer_position(position());
On 2016/09/27 06:49:21, marja wrote:
> IMO none of the Parser impl funcs should call position(), but they should get
> passed all the data they need as parameters. This also makes it less fragile
wrt
> when exactly the impl func is called. Wdyt?

Yes, I agree.  Although position() is a const method (just reading the
parser/scanner state), it's better to pass this info.  I'm making the change.

The impl methods should never modify the parser/scanner state, I mean the things
visible in parser base.  I believe this is not strictly true now, as the parser
base contains parser-only-specific information in its state and parser impl
methods read and update this all the time.  But, largely, I think that parser
impl methods do not modify the part of the parser base state that corresponds to
"pure parsing".

We could enforce this in a future cleanup by making "pure parsing" fields and
methods private in parser base, and making sure that Impl classes are no
friends.

[nickie, 2016-09-27 10:01:56]

The CQ bit was checked by nikolaos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 10:02:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 10:28:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-27 10:28:36]

Dry run: This issue passed the CQ dry run.

[nickie, 2016-09-28 08:36:36]

The CQ bit was checked by nikolaos@chromium.org

[nickie, 2016-09-28 08:36:36]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org,
marja@chromium.org
Link to the patchset: https://codereview.chromium.org/2368083002/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2016-09-28 08:36:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 08:38:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 08:38:18]

Try jobs failed on following builders:
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/13630)

[nickie, 2016-09-28 08:46:06]

The CQ bit was checked by nikolaos@chromium.org

[nickie, 2016-09-28 08:46:06]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org,
marja@chromium.org
Link to the patchset: https://codereview.chromium.org/2368083002/#ps120001
(title: "Rebase again")

[commit-bot: I haz the power, 2016-09-28 08:46:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 09:12:25]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-28 09:12:36]

Description was changed from

==========
[parser] Refactor of ParseClass* and ParseNativeDeclaration

This patch moves the following parsing method to ParserBase:

- ParseClassDeclaration
- ParseClassLiteral
- ParseNativeDeclaration

R=adamk@chromium.org, marja@chromium.org
BUG=
LOG=N
==========

to

==========
[parser] Refactor of ParseClass* and ParseNativeDeclaration

This patch moves the following parsing method to ParserBase:

- ParseClassDeclaration
- ParseClassLiteral
- ParseNativeDeclaration

R=adamk@chromium.org, marja@chromium.org
BUG=
LOG=N

Committed: https://crrev.com/7818355363b7a66ff7709e33c72bfdef5eb21450
Cr-Commit-Position: refs/heads/master@{#39814}
==========

[commit-bot: I haz the power, 2016-09-28 09:12:37]

Patchset 7 (id:??) landed as
https://crrev.com/7818355363b7a66ff7709e33c72bfdef5eb21450
Cr-Commit-Position: refs/heads/master@{#39814}

[Toon Verwaest, 2016-09-28 11:15:53]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2380663002/ by verwaest@chromium.org.

The reason for reverting is: Blocks reverting
https://codereview.chromium.org/2368313002.

[nickie, 2016-09-28 11:59:49]

Description was changed from

==========
[parser] Refactor of ParseClass* and ParseNativeDeclaration

This patch moves the following parsing method to ParserBase:

- ParseClassDeclaration
- ParseClassLiteral
- ParseNativeDeclaration

R=adamk@chromium.org, marja@chromium.org
BUG=
LOG=N

Committed: https://crrev.com/7818355363b7a66ff7709e33c72bfdef5eb21450
Cr-Commit-Position: refs/heads/master@{#39814}
==========

to

==========
[parser] Refactor of ParseClass* and ParseNativeDeclaration

This patch moves the following parsing method to ParserBase:

- ParseClassDeclaration
- ParseClassLiteral
- ParseNativeDeclaration

R=adamk@chromium.org, marja@chromium.org
BUG=
LOG=N

Committed: https://crrev.com/7818355363b7a66ff7709e33c72bfdef5eb21450
Cr-Commit-Position: refs/heads/master@{#39814}
==========

[nickie, 2016-09-28 13:39:03]

On 2016/09/28 11:15:53, Toon Verwaest wrote:
> A revert of this CL (patchset #7 id:120001) has been created in
> https://codereview.chromium.org/2380663002/ by mailto:verwaest@chromium.org.
> 
> The reason for reverting is: Blocks reverting
> https://codereview.chromium.org/2368313002.

I'm trying to reland patchset #7, after 2368313002 was relanded.

[nickie, 2016-09-28 13:39:15]

The CQ bit was checked by nikolaos@chromium.org

[commit-bot: I haz the power, 2016-09-28 13:39:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 13:42:25]

Description was changed from

==========
[parser] Refactor of ParseClass* and ParseNativeDeclaration

This patch moves the following parsing method to ParserBase:

- ParseClassDeclaration
- ParseClassLiteral
- ParseNativeDeclaration

R=adamk@chromium.org, marja@chromium.org
BUG=
LOG=N

Committed: https://crrev.com/7818355363b7a66ff7709e33c72bfdef5eb21450
Cr-Commit-Position: refs/heads/master@{#39814}
==========

to

==========
[parser] Refactor of ParseClass* and ParseNativeDeclaration

This patch moves the following parsing method to ParserBase:

- ParseClassDeclaration
- ParseClassLiteral
- ParseNativeDeclaration

R=adamk@chromium.org, marja@chromium.org
BUG=
LOG=N

Committed: https://crrev.com/7818355363b7a66ff7709e33c72bfdef5eb21450
Cr-Commit-Position: refs/heads/master@{#39814}
==========

[commit-bot: I haz the power, 2016-09-28 13:42:27]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-28 13:42:47]

Description was changed from

==========
[parser] Refactor of ParseClass* and ParseNativeDeclaration

This patch moves the following parsing method to ParserBase:

- ParseClassDeclaration
- ParseClassLiteral
- ParseNativeDeclaration

R=adamk@chromium.org, marja@chromium.org
BUG=
LOG=N

Committed: https://crrev.com/7818355363b7a66ff7709e33c72bfdef5eb21450
Cr-Commit-Position: refs/heads/master@{#39814}
==========

to

==========
[parser] Refactor of ParseClass* and ParseNativeDeclaration

This patch moves the following parsing method to ParserBase:

- ParseClassDeclaration
- ParseClassLiteral
- ParseNativeDeclaration

R=adamk@chromium.org, marja@chromium.org
BUG=
LOG=N

Committed: https://crrev.com/7818355363b7a66ff7709e33c72bfdef5eb21450
Committed: https://crrev.com/da33b67ad796364bf7c990667e32666c0ab1dc48
Cr-Original-Commit-Position: refs/heads/master@{#39814}
Cr-Commit-Position: refs/heads/master@{#39829}
==========

[commit-bot: I haz the power, 2016-09-28 13:42:48]

Patchset 7 (id:??) landed as
https://crrev.com/da33b67ad796364bf7c990667e32666c0ab1dc48
Cr-Commit-Position: refs/heads/master@{#39829}