Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(100)

Issue 1300103005: [parser] disallow language mode directive in body of function with non-simple parameters (Closed)

Created:
4 years, 11 months ago by conradw
Modified:
4 years, 11 months ago
Reviewers:
rossberg
CC:
v8-dev, caitp (gmail)
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[parser] disallow language mode directive in body of function with non-simple parameters TC39 agreed to disallow "use strict" directives in function body when non-simple parameter lists are used. This is a continuation of caitp's CL https://codereview.chromium.org/1281163002/ with some refactorings removed for now. Still TODO: there is a lot of duplication between the is_simple field of FormalParametersBase and the NonSimpleParameter property ExpressionClassifier keeps track of. It should be possible to remove the former with a minor refactoring of arrow function parsing. This will be attempted in a follow-up CL. BUG= LOG=N Committed: https://crrev.com/77394fa05a63a539ac4e6858d99cc85ec6867512 Cr-Commit-Position: refs/heads/master@{#30388}

Patch Set 1 #

Patch Set 2 : alphabetise #

Patch Set 3 : remove logging of parameter simpleness #

Patch Set 4 : rewrite #

Patch Set 5 : rebase #

Patch Set 6 : minor test cleanup #

Total comments: 18

Patch Set 7 : cl feedback #

Patch Set 8 : rebase #

Patch Set 9 : rebase #

Patch Set 10 : flag implications don't work in test suite? #

Unified diffs Side-by-side diffs Delta from patch set Stats (+505 lines, -208 lines) Patch
M src/expression-classifier.h View 1 2 3 4 5 6 3 chunks +26 lines, -6 lines 0 comments Download
M src/messages.h View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M src/parser.h View 1 2 3 4 5 6 7 2 chunks +8 lines, -6 lines 0 comments Download
M src/parser.cc View 1 2 3 4 5 6 7 8 6 chunks +26 lines, -13 lines 0 comments Download
M src/preparser.h View 1 2 3 4 5 6 7 8 12 chunks +23 lines, -7 lines 0 comments Download
M src/preparser.cc View 1 2 3 4 5 6 4 chunks +24 lines, -5 lines 0 comments Download
M src/scopes.h View 1 2 3 4 5 6 7 1 chunk +16 lines, -1 line 0 comments Download
M src/scopes.cc View 1 2 3 4 5 6 7 2 chunks +0 lines, -2 lines 0 comments Download
M test/cctest/test-parsing.cc View 1 2 3 4 5 6 7 8 9 1 chunk +87 lines, -0 lines 0 comments Download
M test/mjsunit/harmony/arrow-rest-params.js View 1 chunk +11 lines, -9 lines 0 comments Download
M test/mjsunit/harmony/default-parameters.js View 1 2 3 4 5 6 7 4 chunks +110 lines, -82 lines 0 comments Download
M test/mjsunit/harmony/destructuring.js View 1 2 3 4 5 6 7 11 chunks +92 lines, -40 lines 0 comments Download
M test/mjsunit/harmony/rest-params.js View 1 2 3 4 5 6 3 chunks +37 lines, -16 lines 0 comments Download
M test/mjsunit/harmony/spread-call-super-property.js View 1 chunk +18 lines, -7 lines 0 comments Download
M test/mjsunit/strong/destructuring.js View 2 chunks +8 lines, -2 lines 0 comments Download
M test/mjsunit/strong/function-arity.js View 1 2 3 7 chunks +15 lines, -10 lines 0 comments Download
M test/mjsunit/strong/literals.js View 1 chunk +2 lines, -2 lines 0 comments Download

Messages

Total messages: 22 (8 generated)
conradw
PTAL
4 years, 11 months ago (2015-08-21 12:27:04 UTC) #2
rossberg
You might want to consider changing the CL title, because the actual revert is not ...
4 years, 11 months ago (2015-08-21 12:40:16 UTC) #3
conradw
On 2015/08/21 12:40:16, rossberg wrote: > You might want to consider changing the CL title, ...
4 years, 11 months ago (2015-08-21 12:43:27 UTC) #4
rossberg
https://codereview.chromium.org/1300103005/diff/100001/src/expression-classifier.h File src/expression-classifier.h (right): https://codereview.chromium.org/1300103005/diff/100001/src/expression-classifier.h#newcode235 src/expression-classifier.h:235: // parameter Nit: period https://codereview.chromium.org/1300103005/diff/100001/src/preparser.cc File src/preparser.cc (right): https://codereview.chromium.org/1300103005/diff/100001/src/preparser.cc#newcode270 ...
4 years, 11 months ago (2015-08-24 13:11:08 UTC) #5
conradw
https://codereview.chromium.org/1300103005/diff/100001/src/expression-classifier.h File src/expression-classifier.h (right): https://codereview.chromium.org/1300103005/diff/100001/src/expression-classifier.h#newcode235 src/expression-classifier.h:235: // parameter On 2015/08/24 13:11:08, rossberg wrote: > Nit: ...
4 years, 11 months ago (2015-08-25 11:31:21 UTC) #6
rossberg
lgtm
4 years, 11 months ago (2015-08-25 12:35:32 UTC) #7
conradw
On 2015/08/25 12:35:32, rossberg wrote: > lgtm this will need to wait for the other ...
4 years, 11 months ago (2015-08-25 12:36:06 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1300103005/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1300103005/120001
4 years, 11 months ago (2015-08-26 09:41:44 UTC) #10
commit-bot: I haz the power
Try jobs failed on following builders: v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/7345) v8_linux_arm_rel on tryserver.v8 (JOB_FAILED, ...
4 years, 11 months ago (2015-08-26 09:42:38 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1300103005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1300103005/140001
4 years, 11 months ago (2015-08-26 13:56:10 UTC) #15
commit-bot: I haz the power
Try jobs failed on following builders: v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/8987)
4 years, 11 months ago (2015-08-26 14:09:04 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1300103005/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1300103005/180001
4 years, 11 months ago (2015-08-26 14:50:19 UTC) #20
commit-bot: I haz the power
Committed patchset #10 (id:180001)
4 years, 11 months ago (2015-08-26 14:59:10 UTC) #21
commit-bot: I haz the power
4 years, 11 months ago (2015-08-26 14:59:28 UTC) #22
Message was sent while issue was closed.
Patchset 10 (id:??) landed as
https://crrev.com/77394fa05a63a539ac4e6858d99cc85ec6867512
Cr-Commit-Position: refs/heads/master@{#30388}

Powered by Google App Engine
This is Rietveld 408576698