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

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

This patch moves the following parsing methods to ParserBase:

- ParseExpressionOrLabelledStatement
- ParseIfStatement
- ParseContinueStatement
- ParseBreakStatement
- ParseReturnStatement
- ParseWithStatement

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

Committed: https://crrev.com/40ba1db51fc4dac7918580111fca857bc0de0ba3
Cr-Commit-Position: refs/heads/master@{#39325}

[nickie, 2016-09-08 13:47:39]

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

[commit-bot: I haz the power, 2016-09-08 13:47: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 14:30:03]

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

[commit-bot: I haz the power, 2016-09-08 14:30:05]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-09-08 18:33:20]

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

https://codereview.chromium.org/2323763002/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:4372: labels = impl()->DeclareLabel(labels, expr,
CHECK_OK);
Can you add an AsIdentifierExpression() method to Parser that does
AsVariableProxy? Basically AsIdentifier() except retaining the expression-ness
(I think this would be a no-op for the PreParser).

That'll make the code inside Parser::DeclareLabel type-safe.

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

https://codereview.chromium.org/2323763002/diff/20001/src/parsing/parser.h#ne...
src/parsing/parser.h:640: return operand != nullptr && !operand->is_this();
Can you add a TODO to remove this null check now that we have the DCHECK? I
agree that we'd rather not change behavior in this patch, but it seems odd.

https://codereview.chromium.org/2323763002/diff/20001/src/parsing/parser.h#ne...
src/parsing/parser.h:668: return expr != nullptr && expr->AsVariableProxy() !=
nullptr &&
Can expr actually be NULL here?

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

https://codereview.chromium.org/2323763002/diff/20001/src/parsing/preparser.h...
src/parsing/preparser.h:451: bool IsEmptyStatement() { return code_ ==
kEmptyStatement; }
Is this only called by IsNullOrEmptyStatement()? The two callers of that both
look suspicious to me, when could they return NULL? Having both Null and Empty
in the preparser seems possibly confusing so if we could get away without it I'd
be happier.

[marja, 2016-09-09 08:45:00]

Q: why do we need to care about IsJumpStatement in the PreParser?

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

https://codereview.chromium.org/2323763002/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:1748: return_value =
impl()->BuildIteratorResult(return_value, true);
Why impl() here?

https://codereview.chromium.org/2323763002/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:1751: impl()->BuildResolvePromise(return_value,
return_value->position());
Ditto

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

https://codereview.chromium.org/2323763002/diff/20001/src/parsing/parser.h#ne...
src/parsing/parser.h:668: return expr != nullptr && expr->AsVariableProxy() !=
nullptr &&
Style nit:

How about:

expr->IsVariableProxy() && expr->AsVariableProxy()-> ...

[nickie, 2016-09-09 09:42:27]

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

https://codereview.chromium.org/2323763002/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:4372: labels = impl()->DeclareLabel(labels, expr,
CHECK_OK);
On 2016/09/08 18:33:19, adamk wrote:
> Can you add an AsIdentifierExpression() method to Parser that does
> AsVariableProxy? Basically AsIdentifier() except retaining the expression-ness
> (I think this would be a no-op for the PreParser).
> 
> That'll make the code inside Parser::DeclareLabel type-safe.

Done.

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

https://codereview.chromium.org/2323763002/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:1748: return_value =
impl()->BuildIteratorResult(return_value, true);
On 2016/09/09 08:44:59, marja wrote:
> Why impl() here?

Right, my bad.  I tried to keep this in parser base, then decided to move it
here.  Nice catch!  :-)

BTW, we have a couple more in ParseClassLiteral (unrelated to this CL), but I'm
leaving them for now, as that will be refactored soon.

https://codereview.chromium.org/2323763002/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:1751: impl()->BuildResolvePromise(return_value,
return_value->position());
On 2016/09/09 08:44:59, marja wrote:
> Ditto

Done.

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

https://codereview.chromium.org/2323763002/diff/20001/src/parsing/parser.h#ne...
src/parsing/parser.h:640: return operand != nullptr && !operand->is_this();
On 2016/09/08 18:33:19, adamk wrote:
> Can you add a TODO to remove this null check now that we have the DCHECK? I
> agree that we'd rather not change behavior in this patch, but it seems odd.

The DCHECK is for a non-existent AST node.  I think I introduced it here and
removed a check from some call sites.  It's definitely good here.

The check for "operand != nullptr" will fail if the expression is not a variable
proxy.  In this case, the function should return false.  We want to keep this.

https://codereview.chromium.org/2323763002/diff/20001/src/parsing/parser.h#ne...
src/parsing/parser.h:668: return expr != nullptr && expr->AsVariableProxy() !=
nullptr &&
On 2016/09/09 08:44:59, marja wrote:
> Style nit:
> 
> How about:
> 
> expr->IsVariableProxy() && expr->AsVariableProxy()-> ...

Done.  I also removed the !scanner()->literal_contains_escapes() and put it back
to the call site, where it belongs.  This method should not have unnecessary
side effects.

https://codereview.chromium.org/2323763002/diff/20001/src/parsing/parser.h#ne...
src/parsing/parser.h:668: return expr != nullptr && expr->AsVariableProxy() !=
nullptr &&
On 2016/09/08 18:33:19, adamk wrote:
> Can expr actually be NULL here?

The DCHECK_NOT_NULL that I just introduced says no...  :-)
But we may not want to change the behavior.  Should I just add a TODO?

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

https://codereview.chromium.org/2323763002/diff/20001/src/parsing/preparser.h...
src/parsing/preparser.h:451: bool IsEmptyStatement() { return code_ ==
kEmptyStatement; }
On 2016/09/08 18:33:19, adamk wrote:
> Is this only called by IsNullOrEmptyStatement()? The two callers of that both
> look suspicious to me, when could they return NULL? Having both Null and Empty
> in the preparser seems possibly confusing so if we could get away without it
I'd
> be happier.

I think the confusion was originally created by "EmptyExpression", which the
parser implements as a nullptr.  This should rather be a "NullExpression" as
there's no such thing as an empty expression.

On the other hand, there's need for a "NullStatement" (which the parser
implements as a nullptr); this is returned, e.g., for *ok == false, or in other
exceptional cases.  And there's "EmptyStatement", which is a valid empty
statement (just the semicolon).

IsNullOrEmptyStatement() is used when checking the directive prologue.  I
thought it was not necessary there, but if you remove it some tests fail.  It
could be replaced there by "IsNullStatement() || IsEmptyStatement()", if you
prefer.

[marja, 2016-09-09 10:41:29]

LGTM

Re: IsNullOrEmpty, maybe IsNull || IsEmpty is cleaner

[nickie, 2016-09-09 10:42:15]

On 2016/09/09 08:45:00, marja wrote:
> Q: why do we need to care about IsJumpStatement in the PreParser?

(Sorry, I forgot to answer this one.)

I'm afraid I don't know the answer.  The current preparser seems to keep track
of jump statements.  I'm not sure if and how this information is used.  If this
is not needed, we can simplify the preparser in a subsequent CL.

[adamk, 2016-09-09 17:14:47]

lgtm

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

https://codereview.chromium.org/2323763002/diff/20001/src/parsing/parser.h#ne...
src/parsing/parser.h:640: return operand != nullptr && !operand->is_this();
On 2016/09/09 09:42:27, nickie wrote:
> On 2016/09/08 18:33:19, adamk wrote:
> > Can you add a TODO to remove this null check now that we have the DCHECK? I
> > agree that we'd rather not change behavior in this patch, but it seems odd.
> 
> The DCHECK is for a non-existent AST node.  I think I introduced it here and
> removed a check from some call sites.  It's definitely good here.
> 
> The check for "operand != nullptr" will fail if the expression is not a
variable
> proxy.  In this case, the function should return false.  We want to keep this.

Ah, indeed, I forgot how AsWhatever works, this is fine.

https://codereview.chromium.org/2323763002/diff/20001/src/parsing/parser.h#ne...
src/parsing/parser.h:668: return expr != nullptr && expr->AsVariableProxy() !=
nullptr &&
On 2016/09/09 09:42:27, nickie wrote:
> On 2016/09/08 18:33:19, adamk wrote:
> > Can expr actually be NULL here?
> 
> The DCHECK_NOT_NULL that I just introduced says no...  :-)
> But we may not want to change the behavior.  Should I just add a TODO?

I'm fine without a TODO, this has very few callers so I think it's pretty safe
to change.

[nickie, 2016-09-10 16:09:54]

On 2016/09/09 10:41:29, marja wrote:
> Re: IsNullOrEmpty, maybe IsNull || IsEmpty is cleaner

OK, I agree.  I'm changing that too.

[nickie, 2016-09-10 16:19:19]

The CQ bit was checked by nikolaos@chromium.org

[nickie, 2016-09-10 16:19:20]

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/2323763002/#ps60001
(title: "Last changes")

[commit-bot: I haz the power, 2016-09-10 16:19:30]

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 16:20:39]

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

[commit-bot: I haz the power, 2016-09-10 16:20:41]

Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  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_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/8595)

[nickie, 2016-09-10 16:40:20]

The CQ bit was checked by nikolaos@chromium.org

[nickie, 2016-09-10 16:40:20]

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

[commit-bot: I haz the power, 2016-09-10 16:40:28]

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 17:04:59]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-10 17:05:44]

Description was changed from

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

This patch moves the following parsing methods to ParserBase:

- ParseExpressionOrLabelledStatement
- ParseIfStatement
- ParseContinueStatement
- ParseBreakStatement
- ParseReturnStatement
- ParseWithStatement

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

to

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

This patch moves the following parsing methods to ParserBase:

- ParseExpressionOrLabelledStatement
- ParseIfStatement
- ParseContinueStatement
- ParseBreakStatement
- ParseReturnStatement
- ParseWithStatement

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

Committed: https://crrev.com/40ba1db51fc4dac7918580111fca857bc0de0ba3
Cr-Commit-Position: refs/heads/master@{#39325}
==========

[commit-bot: I haz the power, 2016-09-10 17:05:44]

Patchset 5 (id:??) landed as
https://crrev.com/40ba1db51fc4dac7918580111fca857bc0de0ba3
Cr-Commit-Position: refs/heads/master@{#39325}