[parser] Refactor of Parse*Statement*, part 1

[parser] Refactor of Parse*Statement*, part 1

This patch moves the following parsing methods to ParserBase:

- ParseStatementList
- ParseStatementListItem
- ParseStatement
- ParseSubStatement (subsumed in ParseStatement)
- ParseStatementAsUnlabeled

It also refactors the Target and TargetScope objects, used by the
parser.

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

Committed: https://crrev.com/df29f3fda25660075a273cc27ad9f7787f321072
Committed: https://crrev.com/f30075bb7014b69ecb931aad1c3467e88dd2f887
Cr-Original-Commit-Position: refs/heads/master@{#39167}
Cr-Commit-Position: refs/heads/master@{#39175}

[nickie, 2016-09-02 16:23:20]

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

[commit-bot: I haz the power, 2016-09-02 16:23:30]

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-02 16:46:43]

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

[commit-bot: I haz the power, 2016-09-02 16:46:44]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-09-02 17:58:24]

I find this CL very hard to review, since it'd basically involve manually
diffing between parser.cc and parser-base.h; is this mostly a straight copy?

[adamk, 2016-09-02 17:58:44]

basically rubberstamp lgtm if this is indeed just a copy (the Target stuff is
fine)

[nickie, 2016-09-04 12:54:20]

On 2016/09/02 17:58:24, adamk wrote:
> I find this CL very hard to review, since it'd basically involve manually
> diffing between parser.cc and parser-base.h; is this mostly a straight copy?

Quick answer, yes, depending on your definition of "mostly".  :-)
A copy of the parser's methods, and then some changes that make them behave
as the preparser's ones for the case of the preparser (the tricky part).

You're right, this is hard to review and there are going to be a number of
CLs like this one.  Next time, I'll upload a first (broken) patch set that
just moves the methods from parser to parser-base, then I'll make the real
changes in a second patch set.  But still, this will not show how the changes
relate to the preparser methods.  I'm afraid there's no easy reviewing for
this sort of refactoring.

[marja, 2016-09-05 08:14:33]

lgtm % comments

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

https://codereview.chromium.org/2307073002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:1191: static const int kLazyParseTrialLimit = 200;
This const declaration could use a short comment here...

https://codereview.chromium.org/2307073002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:1195: USE(result);  // The result is just used in
debug modes.
... this comment, on the other hand, is not very useful :)

https://codereview.chromium.org/2307073002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:4063: return impl()->ParseHoistableDeclaration(NULL,
false, ok);
Nit: NULL -> nullptr in several places

https://codereview.chromium.org/2307073002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:4142: StatementT statement =
ParseStatementAsUnlabelled(labels, CHECK_OK);
What does "ParseStatementAsUnlabelled" mean? You're not changing the name in
this CL but maybe you could add a one-liner comment.

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

https://codereview.chromium.org/2307073002/diff/1/src/parsing/parser.h#newcod...
src/parsing/parser.h:715: ast_value_factory()->use_asm_string();
Nit: these 2 functions are almost duplicate, the only difference is the string
we compare against. Refactor maybe?

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

https://codereview.chromium.org/2307073002/diff/1/src/parsing/preparser.cc#ne...
src/parsing/preparser.cc:747: ParseStatementList(body, Token::RBRACE, CHECK_OK);
Maybe something for a follow-up CL: I find it somewhat surprising that body is
passed by value here - in PreParser it's fine since we don't need to collect
statements, but still, it's a bit weird. It would be more intuitive if we passed
a pointer to it, so the parameter type would be StatementListT* in both cases.
That would also signal to the reader than it's the output parameter.

Would that be feasible or does it conflict too much with other preparser-isms
(such as how ExpressionLists are used now)?

At least some of the ExpressionList usages were like this:
ExpressionListT list = impl()->NewExpressionList();
list->Add(..); // NOOP for PreParser

which is just NOOP but not surprising in the context of PreParser, whereas this
pass-by-value construct is surprising.

What do you think?

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

https://codereview.chromium.org/2307073002/diff/1/src/parsing/preparser.h#new...
src/parsing/preparser.h:758: // All ParseXXX functions take as the last argument
an *ok parameter
This comment could move to ParserBase now, it's more relevant there.

[nickie, 2016-09-05 11:29:32]

I'm landing this and I'm also cc:ing Dan, because of three TODO comments related
to use counters that remain to be resolved eventually (see my comment below).

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

https://codereview.chromium.org/2307073002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:1191: static const int kLazyParseTrialLimit = 200;
On 2016/09/05 08:14:33, marja wrote:
> This const declaration could use a short comment here...

Done.  It never had one, so I improvised.

https://codereview.chromium.org/2307073002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:1195: USE(result);  // The result is just used in
debug modes.
On 2016/09/05 08:14:33, marja wrote:
> ... this comment, on the other hand, is not very useful :)

Done.  Removed.

https://codereview.chromium.org/2307073002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:4012: } else {
These were copied from here [1] and history takes us back to littledan (7ff114e)
[2].  @Dan, can you check please if these make sense?  (Both the calls to
RaiseLanguageMode and the comments, here and a few lines below).

[1] https://cs.chromium.org/chromium/src/v8/src/parsing/parser.cc?rcl=0&l=1006
[2] https://codereview.chromium.org/1429173002

https://codereview.chromium.org/2307073002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:4063: return impl()->ParseHoistableDeclaration(NULL,
false, ok);
On 2016/09/05 08:14:33, marja wrote:
> Nit: NULL -> nullptr in several places

Done.

https://codereview.chromium.org/2307073002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:4142: StatementT statement =
ParseStatementAsUnlabelled(labels, CHECK_OK);
On 2016/09/05 08:14:33, marja wrote:
> What does "ParseStatementAsUnlabelled" mean? You're not changing the name in
> this CL but maybe you could add a one-liner comment.

I don't know what it means... :-)
(Offline discussion.)
I'm adding a few lines to the comment above and I'm adding a comment before the
definition of ParseStatementAsUnlabelled.

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

https://codereview.chromium.org/2307073002/diff/1/src/parsing/parser.h#newcod...
src/parsing/parser.h:715: ast_value_factory()->use_asm_string();
On 2016/09/05 08:14:33, marja wrote:
> Nit: these 2 functions are almost duplicate, the only difference is the string
> we compare against. Refactor maybe?

Done.  I merged both with IsStringLiteral.

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

https://codereview.chromium.org/2307073002/diff/1/src/parsing/preparser.cc#ne...
src/parsing/preparser.cc:747: ParseStatementList(body, Token::RBRACE, CHECK_OK);
On 2016/09/05 08:14:33, marja wrote:
> Maybe something for a follow-up CL: I find it somewhat surprising that body is
> passed by value here - in PreParser it's fine since we don't need to collect
> statements, but still, it's a bit weird. It would be more intuitive if we
passed
> a pointer to it, so the parameter type would be StatementListT* in both cases.
> That would also signal to the reader than it's the output parameter.

Yes, you're right.  It starts from the fact that in ParserTypes<Parser> almost
everything is a pointer (and not a const one), whereas in ParserTypes<PreParser>
they're objects.

I'm adding a TODO comment near the declaration of ParseStatementList, so we can
discuss this later, but there may be other similar cases where we use these
pointers to modify the AST.

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

https://codereview.chromium.org/2307073002/diff/1/src/parsing/preparser.h#new...
src/parsing/preparser.h:758: // All ParseXXX functions take as the last argument
an *ok parameter
On 2016/09/05 08:14:33, marja wrote:
> This comment could move to ParserBase now, it's more relevant there.

Done.

[nickie, 2016-09-05 11:40:42]

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

[commit-bot: I haz the power, 2016-09-05 11:40:46]

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-05 12:04:23]

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

[commit-bot: I haz the power, 2016-09-05 12:04:23]

Dry run: This issue passed the CQ dry run.

[nickie, 2016-09-05 12:40:08]

The CQ bit was checked by nikolaos@chromium.org

[nickie, 2016-09-05 12:40:09]

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/2307073002/#ps20001
(title: "Change according to reviewers' comments")

[commit-bot: I haz the power, 2016-09-05 12:40:15]

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-05 12:52:54]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-05 12:53:06]

Description was changed from

==========
[parser] Refactor of Parse*Statement*, part 1

This patch moves the following parsing methods to ParserBase:

- ParseStatementList
- ParseStatementListItem
- ParseStatement
- ParseSubStatement (subsumed in ParseStatement)
- ParseStatementAsUnlabeled

It also refactors the Target and TargetScope objects, used by the
parser.

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

to

==========
[parser] Refactor of Parse*Statement*, part 1

This patch moves the following parsing methods to ParserBase:

- ParseStatementList
- ParseStatementListItem
- ParseStatement
- ParseSubStatement (subsumed in ParseStatement)
- ParseStatementAsUnlabeled

It also refactors the Target and TargetScope objects, used by the
parser.

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

Committed: https://crrev.com/df29f3fda25660075a273cc27ad9f7787f321072
Cr-Commit-Position: refs/heads/master@{#39167}
==========

[commit-bot: I haz the power, 2016-09-05 12:53:07]

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

[nickie, 2016-09-05 12:57:54]

Description was changed from

==========
[parser] Refactor of Parse*Statement*, part 1

This patch moves the following parsing methods to ParserBase:

- ParseStatementList
- ParseStatementListItem
- ParseStatement
- ParseSubStatement (subsumed in ParseStatement)
- ParseStatementAsUnlabeled

It also refactors the Target and TargetScope objects, used by the
parser.

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

Committed: https://crrev.com/df29f3fda25660075a273cc27ad9f7787f321072
Cr-Commit-Position: refs/heads/master@{#39167}
==========

to

==========
[parser] Refactor of Parse*Statement*, part 1

This patch moves the following parsing methods to ParserBase:

- ParseStatementList
- ParseStatementListItem
- ParseStatement
- ParseSubStatement (subsumed in ParseStatement)
- ParseStatementAsUnlabeled

It also refactors the Target and TargetScope objects, used by the
parser.

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

Committed: https://crrev.com/df29f3fda25660075a273cc27ad9f7787f321072
Cr-Commit-Position: refs/heads/master@{#39167}
==========

[nickie, 2016-09-05 12:59:09]

The CQ bit was checked by nikolaos@chromium.org

[nickie, 2016-09-05 12:59:09]

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/2307073002/#ps40001
(title: "Fix weird compilation bug with line continuation in comment")

[commit-bot: I haz the power, 2016-09-05 12:59:15]

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-05 13:00:23]

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

[commit-bot: I haz the power, 2016-09-05 13:00:24]

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

[Michael Achenbach, 2016-09-05 13:05:38]

Patchset #3 (id:40001) has been deleted

[Michael Achenbach, 2016-09-05 13:05:51]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2313703002/ by machenbach@chromium.org.

The reason for reverting is:
https://build.chromium.org/p/client.v8.ports/builders/V8%20Mips%20-%20builder.

[nickie, 2016-09-05 13:09:35]

The CQ bit was checked by nikolaos@chromium.org

[nickie, 2016-09-05 13:09:35]

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/2307073002/#ps60001
(title: "Fix weird compilation bug with line continuation in comment")

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

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-05 13:42:04]

Description was changed from

==========
[parser] Refactor of Parse*Statement*, part 1

This patch moves the following parsing methods to ParserBase:

- ParseStatementList
- ParseStatementListItem
- ParseStatement
- ParseSubStatement (subsumed in ParseStatement)
- ParseStatementAsUnlabeled

It also refactors the Target and TargetScope objects, used by the
parser.

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

Committed: https://crrev.com/df29f3fda25660075a273cc27ad9f7787f321072
Cr-Commit-Position: refs/heads/master@{#39167}
==========

to

==========
[parser] Refactor of Parse*Statement*, part 1

This patch moves the following parsing methods to ParserBase:

- ParseStatementList
- ParseStatementListItem
- ParseStatement
- ParseSubStatement (subsumed in ParseStatement)
- ParseStatementAsUnlabeled

It also refactors the Target and TargetScope objects, used by the
parser.

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

Committed: https://crrev.com/df29f3fda25660075a273cc27ad9f7787f321072
Cr-Commit-Position: refs/heads/master@{#39167}
==========

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

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-09-05 13:42:50]

Description was changed from

==========
[parser] Refactor of Parse*Statement*, part 1

This patch moves the following parsing methods to ParserBase:

- ParseStatementList
- ParseStatementListItem
- ParseStatement
- ParseSubStatement (subsumed in ParseStatement)
- ParseStatementAsUnlabeled

It also refactors the Target and TargetScope objects, used by the
parser.

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

Committed: https://crrev.com/df29f3fda25660075a273cc27ad9f7787f321072
Cr-Commit-Position: refs/heads/master@{#39167}
==========

to

==========
[parser] Refactor of Parse*Statement*, part 1

This patch moves the following parsing methods to ParserBase:

- ParseStatementList
- ParseStatementListItem
- ParseStatement
- ParseSubStatement (subsumed in ParseStatement)
- ParseStatementAsUnlabeled

It also refactors the Target and TargetScope objects, used by the
parser.

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

Committed: https://crrev.com/df29f3fda25660075a273cc27ad9f7787f321072
Committed: https://crrev.com/f30075bb7014b69ecb931aad1c3467e88dd2f887
Cr-Original-Commit-Position: refs/heads/master@{#39167}
Cr-Commit-Position: refs/heads/master@{#39175}
==========

[commit-bot: I haz the power, 2016-09-05 13:42:51]

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

[Dan Ehrenberg, 2016-09-06 16:57:02]

littledan@chromium.org changed reviewers:
+ littledan@chromium.org

[Dan Ehrenberg, 2016-09-06 16:57:03]

Lgtm; I think the TODOs about language mode can be deleted.

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

https://codereview.chromium.org/2307073002/diff/60001/src/parsing/parser-base...
src/parsing/parser-base.h:4026: RaiseLanguageMode(SLOPPY);
The comparison logic in RaiseLanguageMode which you ported forward should
prevent the wrong UseCounter from being incremented. However, since strong mode
was deleted, maybe this logic can be simplified in a further patch.

[nickie, 2016-09-08 08:58:37]

On 2016/09/06 16:57:03, Dan Ehrenberg wrote:
> Lgtm; I think the TODOs about language mode can be deleted.
> 
>
https://codereview.chromium.org/2307073002/diff/60001/src/parsing/parser-base.h
> File src/parsing/parser-base.h (right):
> 
>
https://codereview.chromium.org/2307073002/diff/60001/src/parsing/parser-base...
> src/parsing/parser-base.h:4026: RaiseLanguageMode(SLOPPY);
> The comparison logic in RaiseLanguageMode which you ported forward should
> prevent the wrong UseCounter from being incremented. However, since strong
mode
> was deleted, maybe this logic can be simplified in a further patch.

My concern here was that calling RaiseLanguageMode(SLOPPY) results in
++use_counts_[v8::Isolate::kSloppyMode], unless the mode was already strict in
which case it results in ++use_counts_[v8::Isolate::kStrictMode].  This means
that if you have a function like:

    function f() {
      'use strict';
      'some string';
      'some other string';
      'some meaningless_expression statement'[3];
      some_statement();
      some_other_statement();
    }

then (as far as I understand) the usage counter for kStrictMode will be
increased six times:

1. when 'use strict' is seen (this is the only one that makes sense to me)
2. when 'some string' is seen and RaiseLanguageMode(SLOPPY) is called from line
4026.
3. similarly for 'some other string'.
4. when 'some meaningless_expression statement'[3] is seen and
RaiseLanguageMode(SLOPPY) is called from line 4031.
5. when some_statement() is seen and RaiseLanguageMode(SLOPPY) is called from
line 4035.
6. similarly for some_other_statement() and every StatementListItem that follows
it.

If 'use strict' was missing or replaced with some other string, exactly the same
would happen, only the usage counter for kSloppyMode would be increased.

I don't know the logic of usage counters but is this really what we want here? 
If you ask me, all three RaiseLanguageMode(SLOPPY) in lines 4026, 4031 and 4035
should be removed, or at least (if we need them for some obscure reason) they
should not trigger the increase of the usage counter.

[nickie, 2016-09-16 10:38:47]

On 2016/09/08 08:58:37, nickie wrote:
> I don't know the logic of usage counters but is this really what we want here?

> If you ask me, all three RaiseLanguageMode(SLOPPY) in lines 4026, 4031 and
4035
> should be removed, or at least (if we need them for some obscure reason) they
> should not trigger the increase of the usage counter.

Reminder, although this CL is long closed.  Should I do something about these
usage counters?  My feeling is we're counting wrong here, unless I don't
understand what we want to count.  Either way, this should be fixed or the TODOs
should be removed.

[Dan Ehrenberg, 2016-09-16 11:49:28]

On 2016/09/16 at 10:38:47, nikolaos wrote:
> On 2016/09/08 08:58:37, nickie wrote:
> > I don't know the logic of usage counters but is this really what we want
here? 
> > If you ask me, all three RaiseLanguageMode(SLOPPY) in lines 4026, 4031 and
4035
> > should be removed, or at least (if we need them for some obscure reason)
they
> > should not trigger the increase of the usage counter.
> 
> Reminder, although this CL is long closed.  Should I do something about these
usage counters?  My feeling is we're counting wrong here, unless I don't
understand what we want to count.  Either way, this should be fixed or the TODOs
should be removed.

I explained in the previous comment the logic of why we aren't counting extra
sloppy mode occurrences: there is logic in RaiseLanguageMode that will switch it
to strict mode before the counter. Anyway, maybe counting sloppy mode is not so
useful anyway; I would not oppose removing it.

[nickie, 2016-09-16 12:01:51]

On 2016/09/16 11:49:28, Dan Ehrenberg wrote:
> I explained in the previous comment the logic of why we aren't counting extra
> sloppy mode occurrences: there is logic in RaiseLanguageMode that will switch
it
> to strict mode before the counter. Anyway, maybe counting sloppy mode is not
so
> useful anyway; I would not oppose removing it.

Dan, are you sure about this?  I saw RaiseLanguageMode and it's just careful
enough not to switch to sloppy mode if the current mode was already strict.  In
any case though, SetLanguageMode is called and that increases the respective
usage counter.  So, I think that what I'm describing in my comment #38 is a real
scenario.  Is this what we want?

[Dan Ehrenberg, 2016-09-16 12:06:58]

On 2016/09/16 at 12:01:51, nikolaos wrote:
> On 2016/09/16 11:49:28, Dan Ehrenberg wrote:
> > I explained in the previous comment the logic of why we aren't counting
extra
> > sloppy mode occurrences: there is logic in RaiseLanguageMode that will
switch it
> > to strict mode before the counter. Anyway, maybe counting sloppy mode is not
so
> > useful anyway; I would not oppose removing it.
> 
> Dan, are you sure about this?  I saw RaiseLanguageMode and it's just careful
enough not to switch to sloppy mode if the current mode was already strict.  In
any case though, SetLanguageMode is called and that increases the respective
usage counter.  So, I think that what I'm describing in my comment #38 is a real
scenario.  Is this what we want?

The counter is only collected for zero/nonzero purposes; over counting does not
matter.

[nickie, 2016-09-16 12:15:46]

On 2016/09/16 12:06:58, Dan Ehrenberg wrote:
> The counter is only collected for zero/nonzero purposes; over counting does
not
> matter.

OK, I'm removing the TODO comments then.
Thanks Dan for seeing this.