[parser] Refactor ParseForStatement.

[parser] Refactor ParseForStatement.

It was a scary function which handled all possible old-fashioned and
for-each statements at one go. Split it to multiple smaller functions
and made the top level logic clearer.

BUG=

Review-Url: https://codereview.chromium.org/2645353002
Cr-Commit-Position: refs/heads/master@{#42627}
Committed: https://chromium.googlesource.com/v8/v8/+/db1de41a62dc90dd853e0e73d941d72fae4ca8f1

[marja, 2017-01-23 10:56:43]

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

[commit-bot: I haz the power, 2017-01-23 10:56:53]

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

[marja, 2017-01-23 11:00:18]

marja@chromium.org changed reviewers:
+ adamk@chromium.org, caitp@igalia.com, nikolaos@chromium.org

[marja, 2017-01-23 11:00:19]

ptal

re: discussion in https://codereview.chromium.org/2637403008/

A more detailed description of the changes (sorry, refactoring is horrible to
review):

The problems of ParseForStatement:
- Long: 300 lines
- Several long if statements
- Deeply nested code
-> when seeing "else" it's not clear which if it binds to
- Weird top-level structure:

if (not semicolon) {
  if (var, let or const) {
    if (for each) {
      // so much code!
    } else {
      // was not for each
    }
  } else {
    // was not var, let or const
  }
}
// standard for statement + fallthrough from the above

In this CL:
- Split long code blocks into 3 functions:
- Made the overall structure clearer:

if (var, let or const) {
  if (for each) {
    return ParseForEachStatementWithInitializers();
  }
  // was not for each
} else if (not semicolon) {
  // was not var, let or const
}

Now the overall structure is shorter, so this logic is visible on one screen!

- Reduced the general indentation level by converting:

if (one) {
  if (two) {
    return 1;
  } else {
    return 2;
  }
} else {
  return 3;
}

into

if (one) {
  if (two) {
    return 1;
  }
  return 2;
}
return 3;

[commit-bot: I haz the power, 2017-01-23 11:41:55]

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

[commit-bot: I haz the power, 2017-01-23 11:41:55]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/19698)
  v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)

[caitp, 2017-01-23 13:22:24]

I appreciate how much simpler this version is to follow, so +1 from me.

[adamk, 2017-01-23 18:46:27]

lgtm, agree that this was a long-needed cleanup

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

https://codereview.chromium.org/2645353002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:1313: StatementT ParseStandardForLoop(StatementT init,
int stmt_pos,
Can you give this a little comment that illustrates what "standard for loop"
means? E.g.,

// Parse a C-style for loop: 'for (<init>; <cond>; <step>) { ... }'

[marja, 2017-01-24 09:36:04]

Thanks for review;

The latest patch set contains some minor changes listed below; I think they're
trivial enough so I'm landing this, pls let me know if you have concerns, so
I'll do a follow-up.

1) I changed the param order of ParseStandardForStatement; stmt_pos should be
first since it's the pos of the whole for statement, and then the init might or
might not be there.

2) Fixed the "jump based on uninitialized value"; the previous code defaulted
bound_names_are_lexical to false and only checked
for_info.parsing_result.descriptor.mode if there were declarations. If there
were no declarations, the value is uninitialized (as is the whole
DeclarationDescriptor)!

An alternative fix would've been to initialize DeclarationDescriptor to some
default values which also make bound_names_are_lexical false, but it's kinda
indirect, so I chose the explicit approach.

And it's still fragile that DeclarationDescriptor doesn't have a proper
initializer, but it's independent of this CL.

[marja, 2017-01-24 09:37:29]

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

https://codereview.chromium.org/2645353002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:1313: StatementT ParseStandardForLoop(StatementT init,
int stmt_pos,
On 2017/01/23 18:46:27, adamk wrote:
> Can you give this a little comment that illustrates what "standard for loop"
> means? E.g.,
> 
> // Parse a C-style for loop: 'for (<init>; <cond>; <step>) { ... }'

Done.

[marja, 2017-01-24 09:37:32]

The CQ bit was checked by marja@chromium.org

[marja, 2017-01-24 09:37:33]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/2645353002/#ps20001
(title: "code review (adamk@ and uninitialized value fix)")

[commit-bot: I haz the power, 2017-01-24 09:37:43]

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

[nickie, 2017-01-24 10:02:48]

This is indeed a big improvement.
There may be room for more refactoring in the specialized methods.
Thanks, Marja.

[commit-bot: I haz the power, 2017-01-24 10:17:03]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1485250652849780,
"parent_rev": "f94c7e8f1a951f6f54a5f629b872404ea9bd16c1", "commit_rev":
"db1de41a62dc90dd853e0e73d941d72fae4ca8f1"}

[commit-bot: I haz the power, 2017-01-24 10:17:10]

Description was changed from

==========
[parser] Refactor ParseForStatement.

It was a scary function which handled all possible old-fashioned and
for-each statements at one go. Split it to multiple smaller functions
and made the top level logic clearer.

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

to

==========
[parser] Refactor ParseForStatement.

It was a scary function which handled all possible old-fashioned and
for-each statements at one go. Split it to multiple smaller functions
and made the top level logic clearer.

BUG=

Review-Url: https://codereview.chromium.org/2645353002
Cr-Commit-Position: refs/heads/master@{#42627}
Committed:
https://chromium.googlesource.com/v8/v8/+/db1de41a62dc90dd853e0e73d941d72fae4...
==========

[commit-bot: I haz the power, 2017-01-24 10:17:12]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/v8/v8/+/db1de41a62dc90dd853e0e73d941d72fae4...