Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(511)

Issue 1429983002: [es6] early error when Identifier is an escaped reserved word (Closed)

Created:
5 years, 1 month ago by caitp (gmail)
Modified:
5 years, 1 month ago
CC:
v8-reviews_googlegroups.com
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[es6] early error when Identifier is an escaped reserved word Per http://tc39.github.io/ecma262/#sec-identifiers-static-semantics-early-errors (13.2.2), make it a SyntaxError if an Identifier has the same StringValue as a ReservedWord. BUG=v8:2222, v8:1972 LOG=N R=adamk@chromium.org, rossberg@chromium.org, wingo@chromium.org Committed: https://crrev.com/5bf360ef5773e5ceecc3b55e349db5a45b831d1d Cr-Commit-Position: refs/heads/master@{#32052}

Patch Set 1 #

Total comments: 31

Patch Set 2 : Bunch of comments addressed + mozilla/test262 statuses updated #

Total comments: 5

Patch Set 3 : Fix test262.status #

Patch Set 4 : More scanner-oriented version #

Patch Set 5 : Remove unneeded preparser change #

Patch Set 6 : Cosmetic fixup 1 #

Total comments: 14

Patch Set 7 : Style nits #

Patch Set 8 : rebase #

Total comments: 15

Patch Set 9 : quick rebase #

Patch Set 10 : Comments addressed + some strictness added #

Patch Set 11 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+309 lines, -149 lines) Patch
M src/messages.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M src/preparser.h View 1 2 3 4 5 6 7 8 9 10 12 chunks +49 lines, -7 lines 0 comments Download
M src/scanner.h View 1 2 3 3 chunks +15 lines, -8 lines 0 comments Download
M src/scanner.cc View 1 2 3 4 5 6 7 8 9 7 chunks +37 lines, -27 lines 0 comments Download
M src/token.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +3 lines, -0 lines 0 comments Download
M test/cctest/test-parsing.cc View 1 2 3 4 5 6 7 8 9 11 chunks +201 lines, -35 lines 0 comments Download
M test/mozilla/mozilla.status View 1 2 3 4 5 6 7 8 9 10 1 chunk +3 lines, -0 lines 0 comments Download
M test/test262/test262.status View 1 2 3 4 5 6 7 8 9 10 1 chunk +0 lines, -72 lines 0 comments Download

Messages

Total messages: 41 (6 generated)
caitp (gmail)
PTAL --- It's unlikely to break anything other than potential attack surfaces (as mentioned by ...
5 years, 1 month ago (2015-11-03 17:19:13 UTC) #1
adamk
See also https://code.google.com/p/v8/issues/detail?id=1972. Does this fix all the test262 failures under language/identifiers/*escape*?
5 years, 1 month ago (2015-11-03 21:27:24 UTC) #3
caitp (gmail)
On 2015/11/03 21:27:24, adamk wrote: > See also https://code.google.com/p/v8/issues/detail?id=1972. Does this fix all > the ...
5 years, 1 month ago (2015-11-03 21:30:53 UTC) #4
caitp (gmail)
On 2015/11/03 21:30:53, caitp wrote: > On 2015/11/03 21:27:24, adamk wrote: > > See also ...
5 years, 1 month ago (2015-11-03 21:31:12 UTC) #5
adamk
Not sure how much to worry about performance here, I'd expect that running over identifiers ...
5 years, 1 month ago (2015-11-03 23:00:57 UTC) #6
caitp (gmail)
I've fixed test statuses and added addressed (I think) a bunch of comments. https://codereview.chromium.org/1429983002/diff/1/src/parser.cc File ...
5 years, 1 month ago (2015-11-04 04:49:25 UTC) #8
caitp (gmail)
some self-review / justification for decisions that might seem/be questionable https://codereview.chromium.org/1429983002/diff/20001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1429983002/diff/20001/src/preparser.h#newcode664 ...
5 years, 1 month ago (2015-11-04 05:03:37 UTC) #9
rossberg
Is there a reason this cannot be fully encapsulated in the scanner? It seems like ...
5 years, 1 month ago (2015-11-04 12:18:54 UTC) #10
caitp (gmail)
On 2015/11/04 12:18:54, rossberg wrote: > Is there a reason this cannot be fully encapsulated ...
5 years, 1 month ago (2015-11-04 13:41:53 UTC) #11
rossberg
On 2015/11/04 13:41:53, caitp wrote: > On 2015/11/04 12:18:54, rossberg wrote: > > Is there ...
5 years, 1 month ago (2015-11-04 14:43:57 UTC) #12
caitp (gmail)
On 2015/11/04 14:43:57, rossberg wrote: > On 2015/11/04 13:41:53, caitp wrote: > > On 2015/11/04 ...
5 years, 1 month ago (2015-11-04 14:46:20 UTC) #13
caitp (gmail)
On 2015/11/04 14:46:20, caitp wrote: > On 2015/11/04 14:43:57, rossberg wrote: > > On 2015/11/04 ...
5 years, 1 month ago (2015-11-04 14:55:42 UTC) #14
rossberg
On 2015/11/04 14:46:20, caitp wrote: > On 2015/11/04 14:43:57, rossberg wrote: > > On 2015/11/04 ...
5 years, 1 month ago (2015-11-04 14:57:46 UTC) #15
rossberg
On 2015/11/04 14:55:42, caitp wrote: > Although to be fair, Mozilla treats an escaped "yield" ...
5 years, 1 month ago (2015-11-04 14:59:22 UTC) #16
adamk
I made a Scanner-only version that passes test262 awhile ago: https://codereview.chromium.org/1297253002/ but I was a ...
5 years, 1 month ago (2015-11-04 16:03:32 UTC) #17
caitp (gmail)
On 2015/11/04 16:03:32, adamk wrote: > I made a Scanner-only version that passes test262 awhile ...
5 years, 1 month ago (2015-11-04 16:05:22 UTC) #18
caitp (gmail)
On 2015/11/04 16:05:22, caitp wrote: > On 2015/11/04 16:03:32, adamk wrote: > > I made ...
5 years, 1 month ago (2015-11-04 16:05:51 UTC) #19
caitp (gmail)
On 2015/11/04 16:05:51, caitp wrote: > On 2015/11/04 16:05:22, caitp wrote: > > On 2015/11/04 ...
5 years, 1 month ago (2015-11-04 16:41:02 UTC) #20
adamk
Curious if rossberg likes this one better, even though it still touches the parser (feel ...
5 years, 1 month ago (2015-11-04 23:42:03 UTC) #21
caitp (gmail)
https://codereview.chromium.org/1429983002/diff/100001/src/scanner.cc File src/scanner.cc (right): https://codereview.chromium.org/1429983002/diff/100001/src/scanner.cc#newcode1236 src/scanner.cc:1236: bool escaped = false; On 2015/11/04 23:42:02, adamk wrote: ...
5 years, 1 month ago (2015-11-04 23:54:16 UTC) #22
caitp (gmail)
On 2015/11/04 16:03:32, adamk wrote: > I made a Scanner-only version that passes test262 awhile ...
5 years, 1 month ago (2015-11-05 01:41:58 UTC) #23
adamk
On 2015/11/05 01:41:58, caitp wrote: > On 2015/11/04 16:03:32, adamk wrote: > > I made ...
5 years, 1 month ago (2015-11-05 23:14:07 UTC) #24
rossberg
Yeah, this is probably as encapsulated as we can make it. :( Still have ome ...
5 years, 1 month ago (2015-11-06 13:31:05 UTC) #25
adamk
https://codereview.chromium.org/1429983002/diff/140001/test/cctest/test-parsing.cc File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/1429983002/diff/140001/test/cctest/test-parsing.cc#newcode1546 test/cctest/test-parsing.cc:1546: if (!is_module) { On 2015/11/06 13:31:05, rossberg wrote: > ...
5 years, 1 month ago (2015-11-06 16:01:32 UTC) #26
caitp (gmail)
https://codereview.chromium.org/1429983002/diff/100001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1429983002/diff/100001/src/preparser.h#newcode2059 src/preparser.h:2059: if (next == Token::ESCAPED_STRICT_RESERVED_WORD) { On 2015/11/06 13:31:04, rossberg ...
5 years, 1 month ago (2015-11-06 16:10:30 UTC) #27
caitp (gmail)
message: Addressed a bunch of comments --- i've added a small amount of code for ...
5 years, 1 month ago (2015-11-06 19:08:06 UTC) #28
rossberg
On 2015/11/06 19:08:06, caitp wrote: > There also seems to be a bug which was ...
5 years, 1 month ago (2015-11-10 08:11:15 UTC) #29
caitp (gmail)
On 2015/11/10 08:11:15, rossberg wrote: > On 2015/11/06 19:08:06, caitp wrote: > > There also ...
5 years, 1 month ago (2015-11-10 11:06:30 UTC) #30
adamk
Ping, rossberg? Are there remaining concerns about this change?
5 years, 1 month ago (2015-11-17 00:23:44 UTC) #31
rossberg
On 2015/11/17 00:23:44, adamk wrote: > Ping, rossberg? Are there remaining concerns about this change? ...
5 years, 1 month ago (2015-11-17 13:32:38 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1429983002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1429983002/180001
5 years, 1 month ago (2015-11-17 15:20:07 UTC) #34
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/10190) v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED, ...
5 years, 1 month ago (2015-11-17 15:20:49 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1429983002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1429983002/200001
5 years, 1 month ago (2015-11-17 15:28:01 UTC) #39
commit-bot: I haz the power
Committed patchset #11 (id:200001)
5 years, 1 month ago (2015-11-17 15:59:51 UTC) #40
commit-bot: I haz the power
5 years, 1 month ago (2015-11-17 16:00:18 UTC) #41
Message was sent while issue was closed.
Patchset 11 (id:??) landed as
https://crrev.com/5bf360ef5773e5ceecc3b55e349db5a45b831d1d
Cr-Commit-Position: refs/heads/master@{#32052}

Powered by Google App Engine
This is Rietveld 408576698