Move ParseVariableDeclarations to ParserBase.

Move ParseVariableDeclarations to ParserBase.

This enables PreParser to declare variables in the future without
duplicating the parsing logic.

BUG=

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

[marja, 2016-09-01 09:16:16]

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

[commit-bot: I haz the power, 2016-09-01 09:16:25]

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

[marja, 2016-09-01 09:22:02]

marja@chromium.org changed reviewers:
+ nikolaos@google.com

[marja, 2016-09-01 09:22:03]

ptal

I was thinking about the parameter ZoneList<const AstRawString*> names, it's
somewhat not very pre-parser-y. (PreParser doesn't use it; it's passed as is to
impl()->DeclareAndInitializeVariables()) But otoh, PreParser already uses
AstRawString*s as parameters to dummy functions... so maybe it's ok.

[nickie, 2016-09-01 09:48:25]

Description was changed from

==========
Move ParseVariableDeclarations to ParserBase.

This enables PreParser to declare variables in the future without
duplicating the parsing logic.

BUG=
==========

to

==========
Move ParseVariableDeclarations to ParserBase.

This enables PreParser to declare variables in the future without
duplicating the parsing logic.

BUG=
==========

[nickie, 2016-09-01 09:48:25]

nikolaos@chromium.org changed reviewers:
+ nikolaos@chromium.org
- nikolaos@google.com

[nickie, 2016-09-01 10:53:08]

LGTM, with some comments.

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

https://codereview.chromium.org/2297563007/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:942: }
I suppose that by rearranging the parameters and putting "arg = nullptr" in the
end, it should be possible to merge this with the previous one.  But, for
consistency, we'd have to do the same in the impl()-> methods.  Then, it may
turn out that these two are not needed anymore and that we should be calling the
impl()-> methods directly.

In any case, I have no objection with this.

https://codereview.chromium.org/2297563007/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:3432: parsing_result->descriptor.mode = VAR;
Is there a reason why this line is not after 3441, inside the "if"?

https://codereview.chromium.org/2297563007/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:3440: if (peek() == Token::VAR) {
Again a matter of taste, but I'd rather use a "switch (peek())" here.  If you
prefer to keep the "if", I suggest that you turn the conditions to, e.g., "if
(Check(Token::VAR))" and remove the "Consume" calls.

https://codereview.chromium.org/2297563007/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:3468: first_declaration = false;
I think you can get rid of these two lines and the variable "first_declaration"
entirely, if you change the condition of the "do-while" in line 3551 to "while
(Check(Token::COMMA))".

https://codereview.chromium.org/2297563007/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:3486: impl()->PushVariableName(fni_,
impl()->AsIdentifier(pattern));
I think that impl()->PushVariableName does not need the first parameter (am I
right it will always be "fni_"?) and it could also incorporate the the "fni_ !=
nullptr" check.

https://codereview.chromium.org/2297563007/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:3556: DCHECK(*ok);
This line strikes me as a bit odd.  Is there any reason why we check this here
in particular?  It's definitely an invariant of all the parsing methods that,
when they return something meaningful, it is *ok == true.  Maybe we should
enforce it everywhere in the parser, e.g. by having a macro like
"RETURN(init_block);" that does this automatically.

Again, this is food for thought, not for this CL.

[marja, 2016-09-01 11:16:10]

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

https://codereview.chromium.org/2297563007/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:942: }
On 2016/09/01 10:53:08, nickie wrote:
> I suppose that by rearranging the parameters and putting "arg = nullptr" in
the
> end, it should be possible to merge this with the previous one.  But, for
> consistency, we'd have to do the same in the impl()-> methods.  Then, it may
> turn out that these two are not needed anymore and that we should be calling
the
> impl()-> methods directly.
> 
> In any case, I have no objection with this.

Removed this one, calling impl()->ReportMessage directly. Not touching any other
ReportMessage funcs in this CL.

https://codereview.chromium.org/2297563007/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:3432: parsing_result->descriptor.mode = VAR;
On 2016/09/01 10:53:08, nickie wrote:
> Is there a reason why this line is not after 3441, inside the "if"?

Done.

https://codereview.chromium.org/2297563007/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:3440: if (peek() == Token::VAR) {
On 2016/09/01 10:53:08, nickie wrote:
> Again a matter of taste, but I'd rather use a "switch (peek())" here.  If you
> prefer to keep the "if", I suggest that you turn the conditions to, e.g., "if
> (Check(Token::VAR))" and remove the "Consume" calls.

Done. (Replaced w/ switch)

https://codereview.chromium.org/2297563007/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:3468: first_declaration = false;
On 2016/09/01 10:53:08, nickie wrote:
> I think you can get rid of these two lines and the variable
"first_declaration"
> entirely, if you change the condition of the "do-while" in line 3551 to "while
> (Check(Token::COMMA))".

Done.

https://codereview.chromium.org/2297563007/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:3486: impl()->PushVariableName(fni_,
impl()->AsIdentifier(pattern));
On 2016/09/01 10:53:08, nickie wrote:
> I think that impl()->PushVariableName does not need the first parameter (am I
> right it will always be "fni_"?) and it could also incorporate the the "fni_
!=
> nullptr" check.

Leaving this as is, to keep PushLiteralName (already existing) and
PushVariableName uniform. I'll do a follow-up CL to get rid of fni in both.

https://codereview.chromium.org/2297563007/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:3556: DCHECK(*ok);
On 2016/09/01 10:53:08, nickie wrote:
> This line strikes me as a bit odd.  Is there any reason why we check this here
> in particular?  It's definitely an invariant of all the parsing methods that,
> when they return something meaningful, it is *ok == true.  Maybe we should
> enforce it everywhere in the parser, e.g. by having a macro like
> "RETURN(init_block);" that does this automatically.
> 
> Again, this is food for thought, not for this CL.

I think this was just paranoid coding, as we use CHECK_OK above in all places
where *ok could be set to false. Leaving this as is.

[marja, 2016-09-01 11:16:14]

The CQ bit was checked by marja@chromium.org

[marja, 2016-09-01 11:16:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
nikolaos@chromium.org
Link to the patchset: https://codereview.chromium.org/2297563007/#ps40001
(title: "code review (nikolaos@)")

[commit-bot: I haz the power, 2016-09-01 11:16:21]

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-01 11:17:32]

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

[commit-bot: I haz the power, 2016-09-01 11:17:33]

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_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_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/8019)

[marja, 2016-09-01 11:32:45]

The CQ bit was checked by marja@chromium.org

[marja, 2016-09-01 11:32:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
nikolaos@chromium.org
Link to the patchset: https://codereview.chromium.org/2297563007/#ps60001
(title: "rebased")

[commit-bot: I haz the power, 2016-09-01 11:32:51]

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-01 11:56:32]

Description was changed from

==========
Move ParseVariableDeclarations to ParserBase.

This enables PreParser to declare variables in the future without
duplicating the parsing logic.

BUG=
==========

to

==========
Move ParseVariableDeclarations to ParserBase.

This enables PreParser to declare variables in the future without
duplicating the parsing logic.

BUG=
==========

[commit-bot: I haz the power, 2016-09-01 11:56:33]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-01 11:57:03]

Description was changed from

==========
Move ParseVariableDeclarations to ParserBase.

This enables PreParser to declare variables in the future without
duplicating the parsing logic.

BUG=
==========

to

==========
Move ParseVariableDeclarations to ParserBase.

This enables PreParser to declare variables in the future without
duplicating the parsing logic.

BUG=

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

[commit-bot: I haz the power, 2016-09-01 11:57:04]

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