[parser] report errors for invalid binding patterns in async formal parameters

[parser] report errors for invalid binding patterns in async formal parameters

BUG=v8:4483, v8:5190
R=caitp@igalia.com, nikolaos@chromium.org

Committed: https://crrev.com/117fda14016a7b3ed4a254448dcafa3cf43a0e7c
Cr-Commit-Position: refs/heads/master@{#37691}

[Dan Ehrenberg, 2016-07-11 23:20:47]

This patch is my take on Caitlin's patch
https://codereview.chromium.org/2133543003 . It tries to be minimal and just
report errors on async function parameters when they are found, avoiding subtle
changes in which errors are propagated where. WDYT?

[Dan Ehrenberg, 2016-07-11 23:23:04]

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

[commit-bot: I haz the power, 2016-07-11 23:23:09]

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-07-11 23:49:12]

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

[commit-bot: I haz the power, 2016-07-11 23:49:12]

Dry run: This issue passed the CQ dry run.

[nickie, 2016-07-12 13:21:04]

LGTM, modulo my comments.

Caitlin, I cannot find evidence that this patch is semantically different from
yours (2133543003) but perhaps I'm missing the context.

If they are indeed equivalent, I obviously prefer this one as it's much simpler.
 If not, please provide a counter example.

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

https://codereview.chromium.org/2139063002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:2834: CHECK_OK);
is_async is necessarily true here, I suggest that you replace it in line 2833,
for clarity (as you already have in line 2855).

https://codereview.chromium.org/2139063002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:2837: if (!*ok) return this->EmptyExpression();
Why not CHECK_OK in line 2836?

[Dan Ehrenberg, 2016-07-12 19:20:30]

On 2016/07/12 13:21:04, nickie wrote:
> LGTM, modulo my comments.
> 
> Caitlin, I cannot find evidence that this patch is semantically different from
> yours (2133543003) but perhaps I'm missing the context.
> 
> If they are indeed equivalent, I obviously prefer this one as it's much
simpler.
>  If not, please provide a counter example.
> 
> https://codereview.chromium.org/2139063002/diff/1/src/parsing/parser-base.h
> File src/parsing/parser-base.h (right):
> 
>
https://codereview.chromium.org/2139063002/diff/1/src/parsing/parser-base.h#n...
> src/parsing/parser-base.h:2834: CHECK_OK);
> is_async is necessarily true here, I suggest that you replace it in line 2833,
> for clarity (as you already have in line 2855).
> 
>
https://codereview.chromium.org/2139063002/diff/1/src/parsing/parser-base.h#n...
> src/parsing/parser-base.h:2837: if (!*ok) return this->EmptyExpression();
> Why not CHECK_OK in line 2836?

Addressed comments in patch set 2

[Dan Ehrenberg, 2016-07-12 19:20:33]

The CQ bit was checked by littledan@chromium.org

[Dan Ehrenberg, 2016-07-12 19:20:34]

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/2139063002/#ps20001
(title: "Cleanups")

[commit-bot: I haz the power, 2016-07-12 19:20:44]

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-07-12 19:47:51]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-12 19:50:19]

Description was changed from

==========
[parser] report errors for invalid binding patterns in async formal parameters

BUG=v8:4483, v8:5190

R=caitp@igalia.com, nikolaos@chromium.org
==========

to

==========
[parser] report errors for invalid binding patterns in async formal parameters

BUG=v8:4483, v8:5190

R=caitp@igalia.com, nikolaos@chromium.org

Committed: https://crrev.com/117fda14016a7b3ed4a254448dcafa3cf43a0e7c
Cr-Commit-Position: refs/heads/master@{#37691}
==========

[commit-bot: I haz the power, 2016-07-12 19:50:20]

Patchset 2 (id:??) landed as
https://crrev.com/117fda14016a7b3ed4a254448dcafa3cf43a0e7c
Cr-Commit-Position: refs/heads/master@{#37691}