PTAL
http://codereview.chromium.org/7207007/diff/1/src/parser.cc
File src/parser.cc (right):
http://codereview.chromium.org/7207007/diff/1/src/parser.cc#newcode2921
src/parser.cc:2921: Handle<String> name = ParseIdentifierName(CHECK_OK);
Better still: revert the change and just replace FUTURE_RESERVED_WORD by
FUTURE_STRICT_RESERVED_WORD.
On 2011/06/22 20:29:33, Lasse Reichstein wrote:
It seems ParseIdentifier would be more correct here (since it doesn't
allow
everything in the IdentifierName category.
On the other hand, it's just using a more general function for a
sub-case that
has already been identified.
Maybe make a comment to that effect.
http://codereview.chromium.org/7207007/diff/1/src/parser.cc#newcode3861
src/parser.cc:3861: Handle<String> Parser::ParseIdentifier(bool* ok) {
Because they implement a different behavior. And the naming is
consistent:
* ParseIdentifier parses only identifiers that are valid for the current
scope (i.e. it fails on strict reserved words in a strict scope).
* ParseIdentifierOrStrictReservedWord parses a valid identifiers for the
current scope or it parses a strict reserved word.
On 2011/06/22 20:29:33, Lasse Reichstein wrote:
This also (like the next function) parse identifier and strict
reserved words.
Why are they named differently?
http://codereview.chromium.org/7207007/diff/1/src/scanner-base.cc
File src/scanner-base.cc (right):
http://codereview.chromium.org/7207007/diff/1/src/scanner-base.cc#newcode791
src/scanner-base.cc:791: KeywordMatcher::FirstState
KeywordMatcher::first_states_[] = {
On 2011/06/22 20:29:33, Lasse Reichstein wrote:
The declaration of KeywordMatcher in scanner-base.h has a comment
listing the
handled keywords. That should be updated to say that we distinguish
future
reserved and future strict-mode reserved words.
Done.
http://codereview.chromium.org/7207007/diff/1/src/scanner-base.cc#newcode903
src/scanner-base.cc:903: Token::FUTURE_STRICT_RESERVED_WORD )) return;
On 2011/06/22 20:29:33, Lasse Reichstein wrote:
Indent to align with "input". Ditto for other cases below.
Done.
http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reserved_words.js
File test/mjsunit/keywords-and-reserved_words.js (right):
http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reserved_words.js#newcode38
test/mjsunit/keywords-and-reserved_words.js:38: eval("var "+x+";");
On 2011/06/22 20:29:33, Lasse Reichstein wrote:
Spaces around '+'.
Even though I think readability can be argued either way.
Done.
http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reserved_words.js#newcode61
test/mjsunit/keywords-and-reserved_words.js:61: if (!isStrictKeyword(x))
On 2011/06/22 20:29:33, Lasse Reichstein wrote:
Use '{' and '}' when the if statement is not on one line.
Done.
http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reserved_words.js#newcode62
test/mjsunit/keywords-and-reserved_words.js:62: return "ERROR";
On 2011/06/22 20:29:33, Lasse Reichstein wrote:
and indent by 2.
Done.
http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reserved_words.js#newcode67
test/mjsunit/keywords-and-reserved_words.js:67: if (isStrictKeyword(x))
On 2011/06/22 20:29:33, Lasse Reichstein wrote:
ditto here.
Done.
http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reserved_words.js#newcode75
test/mjsunit/keywords-and-reserved_words.js:75: assertEquals ("keyword",
classifyIdentifier(word));
On 2011/06/22 20:29:33, Lasse Reichstein wrote:
Extra space after "assertEquals".
Done.
http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reserved_words.js#newcode79
test/mjsunit/keywords-and-reserved_words.js:79: if(word != "this")
On 2011/06/22 20:29:33, Lasse Reichstein wrote:
Space after "if", '{' and '}' around body.
Done.
http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reserved_words.js#newcode101
test/mjsunit/keywords-and-reserved_words.js:101: assertThrows("var x = {
get foo(" + word + ") { } };", SyntaxError);
This was copied from strict-mode.js. Before I delete it,
what is the reason it was added there in the first place?
On 2011/06/22 20:29:33, Lasse Reichstein wrote:
This is a syntax error for any word. Literal getters cannot have a
formal
parameter.
http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reserved_words.js#newcode117
test/mjsunit/keywords-and-reserved_words.js:117: "double",
"volatile" ];
Done. (That's from the WebKit test. Can't take the credit.)
On 2011/06/22 20:29:33, Lasse Reichstein wrote:
Great that you check the ES3 keywords that are not in ES5!
Worth making a comment about.
http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reserved_words.js#newcode128
test/mjsunit/keywords-and-reserved_words.js:128: "continue",
"return",
On 2011/06/22 20:29:33, Lasse Reichstein wrote:
"const".
It's an ES5 future reserved word, so it should at least be mentioned
there, but
we really treat it as a keyword (not that there is any way to
distinguish the
two).
Done.
http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reserved_words.js#newcode137
test/mjsunit/keywords-and-reserved_words.js:137: "if", "with"
];
This one is nasty. 'var \u0069f;' works but 'eval("var \u0069f;");' does
not. What exactly would you like to test here?
On 2011/06/22 20:29:33, Lasse Reichstein wrote:
Currently we consider \u0069f as a valid identifier (even though it
really
isn't according to the specification).
Maybe we should also test that it works.
http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reserved_words.js#newcode169
test/mjsunit/keywords-and-reserved_words.js:169: assertEquals ("strict",
classifyIdentifier(future_strict_reserved_words[i]));
On 2011/06/22 20:29:33, Lasse Reichstein wrote:
Make a note that the strict mode tests are in strict-mode.js.
Done.
http://codereview.chromium.org/7207007/diff/1/test/mjsunit/strict-mode.js
File test/mjsunit/strict-mode.js (right):
http://codereview.chromium.org/7207007/diff/1/test/mjsunit/strict-mode.js#newcode362
test/mjsunit/strict-mode.js:362: assertThrows("function foo (" + word +
") 'use strict'; {}", SyntaxError);
Moved it outside of the loop.
On 2011/06/22 20:29:33, Lasse Reichstein wrote:
This is just a plain syntax error, unrelated to 'word'.
http://codereview.chromium.org/7207007/diff/1/test/mjsunit/strict-mode.js#newcode374
test/mjsunit/strict-mode.js:374: eval("var x = { set " + word + "
(value) { 'use strict'; } };");
Lines 348-349 are slightly different because they test
property names given by string literals. I would keep both but move
373-374 up.
On 2011/06/22 20:29:33, Lasse Reichstein wrote:
Duplicates line 348 and 349.
http://codereview.chromium.org/7207007/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
