I like this (i.e., it's very close to how I would have done it).
The problems with detecting "use strict" can be fixed without resorting to
extra
passes.
The rest looks well structured and follows the existing conventions well.
http://codereview.chromium.org/6144005/diff/1/src/parser.cc
File src/parser.cc (right):
http://codereview.chromium.org/6144005/diff/1/src/parser.cc#newcode1084
src/parser.cc:1084: if (peek() == Token::STRING) {
As you pointed out, this is too simple (and ECMAScript is rarely simple
:).
The missing parts is checking that the "use strict" is not part of a
larger expression, and that it doesn't contain escapes. There are ways
to do both without rescanning or prescanning.
If an ExpressionStatement *starts* with a string token, and is itself a
single string, then there are no wrapping parentheses.
And if the string literal's start and end position are exactly
strlen("use strict") + 2 apart (content + quotes), then there are no
escapes in the string.
The solution I have come up with is something like (from memory):
Statement* stat;
if (directive_prologue && peek() == Token::STRING) {
var location = scanner_pos();
stat = ParseStatement(NULL, CHECK);
ExpressionStatement* expr_stat = stat->AsExpressionStatement();
if (expr_stat != NULL) {
Handle<Object> literal = expr_stat->handle();
if (literal->IsString()) {
// It's a directive prologue.
Handle<String> prologue = Handle<String>::cast(literal);
if (prologue.Equals(Heap::use_strict_symbol()) &&
position.end_pos - position.beg_pos == 12) {
// Literal location, including quotes, leaves
// no room for escaped characters.
temp_scope_->SetStrict();
}
continue; // Don't add it to the AST.
}
}
directive_prologue = false;
} else {
directive_prologue = false;
stat = ParseStatement(NULL, CHECK);
}
I'm not entirely satisfied with the flow yet :).
http://codereview.chromium.org/6144005/diff/1/src/parser.cc#newcode3252
src/parser.cc:3252: StrictMode strict(&strict_mode_);
As stated earlier, we define the TemporaryScope just above, which has
the same liveness and general meaning (collecting stuff to put into
FunctionLiteral) as the StrictMode here.
http://codereview.chromium.org/6144005/diff/1/src/parser.cc#newcode3344
src/parser.cc:3344: ReportMessageAt(Scanner::Location(position,
start_pos),
Could you store the position of the argument that is "eval" or
"arguments"? I.e., so that if thestored position is
RelocInfo::kNoPosition, then there is none, and otherwise it's the
position of the offending argument.
http://codereview.chromium.org/6144005/diff/1/src/parser.cc#newcode3351
src/parser.cc:3351: "strict_param_dupe", Vector<const char*>::empty());
Also here, could you store the actual position of the (second instance
of) the offending variable name?
http://codereview.chromium.org/6144005/diff/1/src/parser.cc#newcode3354
src/parser.cc:3354: }
For later, I would also put the check for octal numbers here.
I imagine letting the scanner hold the position of the most recent octal
escape or octal number literal, and then testing at this point whether
it lies between start_pos and end_pos.
That will report the last octal number, not the first, but I think it's
better than checking all the time.
http://codereview.chromium.org/6144005/diff/1/src/parser.h
File src/parser.h (right):
http://codereview.chromium.org/6144005/diff/1/src/parser.h#newcode685
src/parser.h:685: bool strict_mode_; // parsing strict mode code
You could (and IMO should) put this bit in the TemporaryScope in
temp_scope_. TemporaryScope collects other things that are put into the
FunctionLiteral as well.
You can set TemporaryScope to automatically inherit the is_strict bit
from its parent the same way the StrictMode class does.
http://codereview.chromium.org/6144005/diff/1/src/scopes.cc
File src/scopes.cc (right):
http://codereview.chromium.org/6144005/diff/1/src/scopes.cc#newcode417
src/scopes.cc:417: HashMap map(Match, &HashMap::DefaultAllocator, length
* 2);
If the length is short (e.g., five or less), just do the quadratic
comparison. We are comparing symbols, so it's pretty quick, and I'm
guessing that the average number of arguments to a function is around
two (could be a fun thing to check :).
http://codereview.chromium.org/6144005/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
