Centralize and standardize logic for ExpressionClassifier accumulation

Centralize and standardize logic for ExpressionClassifier accumulation

Previously the calls to ExpressionClassifier::Accumulate() each chose
slightly different sets of productions to accumulate, and it turned
out that these were in some cases broader than needed and in some
cases less broad.

The existence of some grab-bag production bitmasks like
ExpressionClassifier::ExpressionProductions made this situation more
error-prone (for example, that production was missing AsyncArrowFormalParametersProduction).

This patch removes all "grab-bags" besides AllProductions. In some of
the places where code was using those grab-bags for convenience, it
switches them to use negation of AllProductions. In other, specifically
those having to do with expressions that are disallowed anywhere in
a sub-expression of a parameter list, I've added a new method on
ExpressionClassifier to centralize the logic.

The aforementioned centralization/addition of
AsyncArrowFormalParametersProduction fixes several cases where we were
failing to report an error for 'await' in some contexts; I've added
those test cases.

The patch also narrows all cases to exactly the set or productions
necessary, with a comment on each explaining the choice.

BUG=v8:4483
Committed: https://crrev.com/51c186dd983e542d2ac17835f73df8b1e5c6461e
Cr-Commit-Position: refs/heads/master@{#38918}

[adamk, 2016-08-24 02:11:21]

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

[commit-bot: I haz the power, 2016-08-24 02:11:26]

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-08-24 02:24:35]

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

[commit-bot: I haz the power, 2016-08-24 02:24:35]

Dry run: 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_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[adamk, 2016-08-24 18:22:26]

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

[commit-bot: I haz the power, 2016-08-24 18:22: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-08-24 18:24:00]

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

[commit-bot: I haz the power, 2016-08-24 18:24:01]

Dry run: Try jobs failed on following builders:
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/11540)
  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_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  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_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/22514)

[adamk, 2016-08-24 19:55:47]

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

[commit-bot: I haz the power, 2016-08-24 19:55:54]

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-08-24 19:57:07]

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

[commit-bot: I haz the power, 2016-08-24 19:57:08]

Dry run: Try jobs failed on following builders:
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/11543)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/7604)

[adamk, 2016-08-24 20:05:58]

Description was changed from

==========
WIP: More ExpressionClassifier cleanup and async/await bug fixes
==========

to

==========
Centralize and standardize logic for ExpressionClassifier accumulation

Previously the calls to ExpressionClassifier::Accumulate() each chose
slightly different sets of productions to accumulate, and it turned
out that these were in some cases broader than needed and in some
cases less broad.

The existence of some grab-bag production bitmasks like 
ExpressionClassifier::ExpressionProductions made this situation more
error-prone (for example, that production was missing
AsyncArrowFormalParametersProduction).

This patch removes all "grab-bags" besides AllProductions. In some of
the places where code was using those grab-bags for convenience, it
switches them to use negation of AllProductions. In other, specifically
those having to do with expressions that are disallowed anywhere in
a sub-expression of a parameter list, I've added a new method on
ExpressionClassifier to centralize the logic.

The aforementioned centralization/addition of 
AsyncArrowFormalParametersProduction fixes several cases where we were
failing to report an error for 'await' in some contexts; I've added
those test cases.

The patch also narrows all cases to exactly the set or productions
necessary, with a comment on each explaining the choice.

BUG=v8:4483
==========

[adamk, 2016-08-24 20:14:19]

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

[commit-bot: I haz the power, 2016-08-24 20:14:30]

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

[adamk, 2016-08-24 20:14:42]

adamk@chromium.org changed reviewers:
+ littledan@chromium.org, nikolaos@chromium.org

[commit-bot: I haz the power, 2016-08-24 20:37:52]

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

[commit-bot: I haz the power, 2016-08-24 20:37:53]

Dry run: This issue passed the CQ dry run.

[nickie, 2016-08-25 10:48:00]

LGTM, 1 comment

https://codereview.chromium.org/2271063002/diff/80001/src/parsing/expression-...
File src/parsing/expression-classifier.h (right):

https://codereview.chromium.org/2271063002/diff/80001/src/parsing/expression-...
src/parsing/expression-classifier.h:386: void
AccumulateFormalParameterContainmentErrors(ExpressionClassifier* inner) {
I'm not sure I understand entirely the logic of V8_INLINE, but shouldn't this
method be qualified as such?

[adamk, 2016-08-25 15:45:56]

The CQ bit was checked by adamk@chromium.org

[adamk, 2016-08-25 15:45:56]

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/2271063002/#ps100001
(title: "V8_INLINE")

[commit-bot: I haz the power, 2016-08-25 15:46:04]

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

[adamk, 2016-08-25 15:46:27]

https://codereview.chromium.org/2271063002/diff/80001/src/parsing/expression-...
File src/parsing/expression-classifier.h (right):

https://codereview.chromium.org/2271063002/diff/80001/src/parsing/expression-...
src/parsing/expression-classifier.h:386: void
AccumulateFormalParameterContainmentErrors(ExpressionClassifier* inner) {
On 2016/08/25 10:48:00, nickie wrote:
> I'm not sure I understand entirely the logic of V8_INLINE, but shouldn't this
> method be qualified as such?

Agreed that I don't know of good guidelines for when to use it, but there's
definitely no harm here. Added.

[commit-bot: I haz the power, 2016-08-25 15:47:22]

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

[commit-bot: I haz the power, 2016-08-25 15:47:23]

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_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_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/11626)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  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_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_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/11565)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/7692)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/22622)

[adamk, 2016-08-25 16:22:45]

The CQ bit was checked by adamk@chromium.org

[adamk, 2016-08-25 16:22: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/2271063002/#ps120001
(title: "Rebased")

[commit-bot: I haz the power, 2016-08-25 16:22:54]

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

Description was changed from

==========
Centralize and standardize logic for ExpressionClassifier accumulation

Previously the calls to ExpressionClassifier::Accumulate() each chose
slightly different sets of productions to accumulate, and it turned
out that these were in some cases broader than needed and in some
cases less broad.

The existence of some grab-bag production bitmasks like 
ExpressionClassifier::ExpressionProductions made this situation more
error-prone (for example, that production was missing
AsyncArrowFormalParametersProduction).

This patch removes all "grab-bags" besides AllProductions. In some of
the places where code was using those grab-bags for convenience, it
switches them to use negation of AllProductions. In other, specifically
those having to do with expressions that are disallowed anywhere in
a sub-expression of a parameter list, I've added a new method on
ExpressionClassifier to centralize the logic.

The aforementioned centralization/addition of 
AsyncArrowFormalParametersProduction fixes several cases where we were
failing to report an error for 'await' in some contexts; I've added
those test cases.

The patch also narrows all cases to exactly the set or productions
necessary, with a comment on each explaining the choice.

BUG=v8:4483
==========

to

==========
Centralize and standardize logic for ExpressionClassifier accumulation

Previously the calls to ExpressionClassifier::Accumulate() each chose
slightly different sets of productions to accumulate, and it turned
out that these were in some cases broader than needed and in some
cases less broad.

The existence of some grab-bag production bitmasks like 
ExpressionClassifier::ExpressionProductions made this situation more
error-prone (for example, that production was missing
AsyncArrowFormalParametersProduction).

This patch removes all "grab-bags" besides AllProductions. In some of
the places where code was using those grab-bags for convenience, it
switches them to use negation of AllProductions. In other, specifically
those having to do with expressions that are disallowed anywhere in
a sub-expression of a parameter list, I've added a new method on
ExpressionClassifier to centralize the logic.

The aforementioned centralization/addition of 
AsyncArrowFormalParametersProduction fixes several cases where we were
failing to report an error for 'await' in some contexts; I've added
those test cases.

The patch also narrows all cases to exactly the set or productions
necessary, with a comment on each explaining the choice.

BUG=v8:4483
==========

[commit-bot: I haz the power, 2016-08-25 16:59:17]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-08-25 16:59:52]

Description was changed from

==========
Centralize and standardize logic for ExpressionClassifier accumulation

Previously the calls to ExpressionClassifier::Accumulate() each chose
slightly different sets of productions to accumulate, and it turned
out that these were in some cases broader than needed and in some
cases less broad.

The existence of some grab-bag production bitmasks like 
ExpressionClassifier::ExpressionProductions made this situation more
error-prone (for example, that production was missing
AsyncArrowFormalParametersProduction).

This patch removes all "grab-bags" besides AllProductions. In some of
the places where code was using those grab-bags for convenience, it
switches them to use negation of AllProductions. In other, specifically
those having to do with expressions that are disallowed anywhere in
a sub-expression of a parameter list, I've added a new method on
ExpressionClassifier to centralize the logic.

The aforementioned centralization/addition of 
AsyncArrowFormalParametersProduction fixes several cases where we were
failing to report an error for 'await' in some contexts; I've added
those test cases.

The patch also narrows all cases to exactly the set or productions
necessary, with a comment on each explaining the choice.

BUG=v8:4483
==========

to

==========
Centralize and standardize logic for ExpressionClassifier accumulation

Previously the calls to ExpressionClassifier::Accumulate() each chose
slightly different sets of productions to accumulate, and it turned
out that these were in some cases broader than needed and in some
cases less broad.

The existence of some grab-bag production bitmasks like
ExpressionClassifier::ExpressionProductions made this situation more
error-prone (for example, that production was missing
AsyncArrowFormalParametersProduction).

This patch removes all "grab-bags" besides AllProductions. In some of
the places where code was using those grab-bags for convenience, it
switches them to use negation of AllProductions. In other, specifically
those having to do with expressions that are disallowed anywhere in
a sub-expression of a parameter list, I've added a new method on
ExpressionClassifier to centralize the logic.

The aforementioned centralization/addition of
AsyncArrowFormalParametersProduction fixes several cases where we were
failing to report an error for 'await' in some contexts; I've added
those test cases.

The patch also narrows all cases to exactly the set or productions
necessary, with a comment on each explaining the choice.

BUG=v8:4483

Committed: https://crrev.com/51c186dd983e542d2ac17835f73df8b1e5c6461e
Cr-Commit-Position: refs/heads/master@{#38918}
==========

[commit-bot: I haz the power, 2016-08-25 16:59:53]

Patchset 7 (id:??) landed as
https://crrev.com/51c186dd983e542d2ac17835f73df8b1e5c6461e
Cr-Commit-Position: refs/heads/master@{#38918}