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

Issue 152813005: Make strict more error messages about "eval" and "arguments" less specific. (Closed)

Created:
6 years, 10 months ago by marja
Modified:
6 years, 10 months ago
Reviewers:
ulan
CC:
v8-dev
Visibility:
Public.

Description

Make strict more error messages about "eval" and "arguments" less specific. We used to have error messages which provide context, like "Variable name may not be eval or arguments in strict mode", but for other illegal words we only have non-context specific error messages like "Unexpected reserved word". Providing the context makes the code unnecessarily complex, since every individual place must remember to check for eval or arguments. This CL produces a unified error message ("Unexpected eval or arguments in strict mode"), and puts the error reporting to (Pre)Parser::ParseIdentifier. Notes: - The module feature is so experimental, that I decided to not allow "eval" or "arguments" as module-related identifiers in the strict mode (even though this check wasn't there before). - Unfortunately, there were some inconsistencies, since it was the responsibility of the caller of ParseIdentifier to check "eval" and "arguments" and some places didn't have the check for no good reason. This CL is supposed to keep backward compatibility and *not* introduce any new errors. - ECMA allows "eval" and "arguments" as labels even in strict mode. (Syntax: "LabelledStatement: Identifier : Statement", and no strict mode restrictions on Identifier are listed.) - Tests which compare error message strings will fail, and need to be updated. BUG=3126 LOG=N R=ulan@chromium.org Committed: https://code.google.com/p/v8/source/detail?r=19112

Patch Set 1 #

Patch Set 2 : . #

Patch Set 3 : . #

Patch Set 4 : . #

Total comments: 2

Patch Set 5 : Code review (ulan@) #

Patch Set 6 : Code review (ulan@) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+136 lines, -130 lines) Patch
M src/messages.js View 1 chunk +1 line, -7 lines 0 comments Download
M src/parser.h View 1 2 3 4 2 chunks +1 line, -2 lines 0 comments Download
M src/parser.cc View 1 2 3 4 17 chunks +42 lines, -38 lines 0 comments Download
M src/preparser.h View 1 2 3 4 2 chunks +6 lines, -1 line 0 comments Download
M src/preparser.cc View 1 2 3 4 13 chunks +43 lines, -39 lines 0 comments Download
M test/webkit/fast/js/basic-strict-mode-expected.txt View 3 chunks +43 lines, -43 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
ulan
lgtm https://codereview.chromium.org/152813005/diff/10007/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/152813005/diff/10007/src/parser.cc#newcode986 src/parser.cc:986: Handle<String> name = ParseIdentifier(false, CHECK_OK); I prefer enums ...
6 years, 10 months ago (2014-02-05 16:05:42 UTC) #1
marja
Thanks for comments! https://codereview.chromium.org/152813005/diff/10007/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/152813005/diff/10007/src/parser.cc#newcode986 src/parser.cc:986: Handle<String> name = ParseIdentifier(false, CHECK_OK); On ...
6 years, 10 months ago (2014-02-05 16:18:01 UTC) #2
marja
6 years, 10 months ago (2014-02-05 16:26:57 UTC) #3
Message was sent while issue was closed.
Committed patchset #6 manually as r19112 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698