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

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

This patch moves the following parsing methods to ParserBase:

- ParseDoExpression
- ParseDoWhileStatement
- ParseWhileStatement
- ParseThrowStatement

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

Committed: https://crrev.com/bf85ca531510879c77b3fd707d498ada9677cb74
Cr-Commit-Position: refs/heads/master@{#39326}

[nickie, 2016-09-08 16:34:26]

The bytecode expectation had to change because now the new temporary for the do
expression is generated after the body is parsed.  I don't think there should be
any problem with that.

[nickie, 2016-09-08 16:34:35]

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

[commit-bot: I haz the power, 2016-09-08 16:34:44]

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-08 16:36:16]

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

[commit-bot: I haz the power, 2016-09-08 16:36:17]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/12397)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/12329)

[adamk, 2016-09-08 18:44:47]

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

https://codereview.chromium.org/2321103002/diff/20001/src/parsing/parser.h#ne...
src/parsing/parser.h:409: if (loop != nullptr) loop->Initialize(cond, body);
I don't understand, when can loop be null?

[nickie, 2016-09-09 09:43:37]

BTW, this CL needs rebasing, that's why the bots failed.

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

https://codereview.chromium.org/2321103002/diff/20001/src/parsing/parser.h#ne...
src/parsing/parser.h:409: if (loop != nullptr) loop->Initialize(cond, body);
On 2016/09/08 18:44:47, adamk wrote:
> I don't understand, when can loop be null?

I don't know either, but it was in all call sites and I did not want to change
the behavior in something like this.  In the two call sites related to this CL,
the loop has just been generated by a call to a factory method.  I presume this
will never return a nullptr.  The case of the for loop is more complicated, I
haven't got there yet.

Should I add a TODO comment here?

[marja, 2016-09-09 10:48:03]

lgtm modulo the ongoing discussions w/ adamk@

[adamk, 2016-09-09 17:21:06]

lgtm once InitializeLoop is gone.

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

https://codereview.chromium.org/2321103002/diff/20001/src/parsing/parser.h#ne...
src/parsing/parser.h:409: if (loop != nullptr) loop->Initialize(cond, body);
On 2016/09/09 09:43:37, nickie wrote:
> On 2016/09/08 18:44:47, adamk wrote:
> > I don't understand, when can loop be null?
> 
> I don't know either, but it was in all call sites and I did not want to change
> the behavior in something like this.  In the two call sites related to this
CL,
> the loop has just been generated by a call to a factory method.  I presume
this
> will never return a nullptr.  The case of the for loop is more complicated, I
> haven't got there yet.

Those if statements go back to 2009, and my reading of the code suggests that
the null check is a no-op. As for for loops, they don't even do the null check.

> Should I add a TODO comment here?

I'd rather not add a new method InitializeLoop as part of this refactor along
with a TODO to remove it.

Please just call Initialize() without a check.

[nickie, 2016-09-10 17:41:04]

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

https://codereview.chromium.org/2321103002/diff/20001/src/parsing/parser.h#ne...
src/parsing/parser.h:409: if (loop != nullptr) loop->Initialize(cond, body);
On 2016/09/09 17:21:06, adamk wrote:
> On 2016/09/09 09:43:37, nickie wrote:
> > On 2016/09/08 18:44:47, adamk wrote:
> > > I don't understand, when can loop be null?
> > 
> > I don't know either, but it was in all call sites and I did not want to
change
> > the behavior in something like this.  In the two call sites related to this
> CL,
> > the loop has just been generated by a call to a factory method.  I presume
> this
> > will never return a nullptr.  The case of the for loop is more complicated,
I
> > haven't got there yet.
> 
> Those if statements go back to 2009, and my reading of the code suggests that
> the null check is a no-op. As for for loops, they don't even do the null
check.
> 
> > Should I add a TODO comment here?
> 
> I'd rather not add a new method InitializeLoop as part of this refactor along
> with a TODO to remove it.
> 
> Please just call Initialize() without a check.

Done.  I removed InitializeLoop and added a dummy Initialize method in
PreParserStatement.

[nickie, 2016-09-10 17:41:11]

The CQ bit was checked by nikolaos@chromium.org

[nickie, 2016-09-10 17:41:11]

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/2321103002/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2016-09-10 17:41:22]

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-10 18:05:00]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-10 18:05:34]

Description was changed from

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

This patch moves the following parsing methods to ParserBase:

- ParseDoExpression
- ParseDoWhileStatement
- ParseWhileStatement
- ParseThrowStatement

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

to

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

This patch moves the following parsing methods to ParserBase:

- ParseDoExpression
- ParseDoWhileStatement
- ParseWhileStatement
- ParseThrowStatement

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

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

[commit-bot: I haz the power, 2016-09-10 18:05:35]

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