Thanks for the feedback, Lasse!
Tried to incorporate all of it, in few places I don't know how nervous to be
about additional work now happening when processing non-strict code.
Also adding --strict_mode flag.
Main question - how does one allocate transient memory in the parser? A
memory
that only sometimes is needed and when it is I'd love to free it ideally as
soon
as possible. The code is marked below with a comment so hopefully it'll be
obvious what I am trying to do. For some reason new/delete didn't work even
though the underlying operators did seem to do the right thing.
Thank you!
Martin
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) {
On 2011/01/13 07:18:40, Lasse Reichstein wrote:
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 :).
Done. However, we still need to add the directives to the AST. Consider:
eval("Hello").length
http://codereview.chromium.org/6144005/diff/1/src/parser.cc#newcode3252
src/parser.cc:3252: StrictMode strict(&strict_mode_);
On 2011/01/13 07:18:40, Lasse Reichstein wrote:
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.
Done.
http://codereview.chromium.org/6144005/diff/1/src/parser.cc#newcode3344
src/parser.cc:3344: ReportMessageAt(Scanner::Location(position,
start_pos),
It is definitely possible. I was trying to avoid any unnecessary work
that would affect non-strict mode code. In this case, checking parameter
names for eval/arguments will have to happen regardless of strict mode
because of:
function foo(eval, arguments) { // Seen not in strict mode but must ERR
"use strict";
}
This check is relatively cheap so I will add it.
On 2011/01/13 07:18:40, Lasse Reichstein wrote:
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());
I believe it can be done, also at a relatively small cost to the
non-strict mode parsing.
The only glitch is being able to allocate HashMap temporarily so that
only functions with >N parameters use hashmap for duplicate checks.
How can I allocate such temporary hash map inside V8 parser? I am hoping
to put it onto TemporaryScope as well and allocate it only when needed,
and ideally free it as soon as the arguments have been processed.
On 2011/01/13 07:18:40, Lasse Reichstein wrote:
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: }
Great idea. that will also handle the case where "use strict" is
preceded by another directive that does contain octal literal. Will do
when I implement the octal detection.
On 2011/01/13 07:18:40, Lasse Reichstein wrote:
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
On 2011/01/13 07:18:40, Lasse Reichstein wrote:
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.
Done.
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);
On 2011/01/13 07:18:40, Lasse Reichstein wrote:
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 :).
Done.
http://codereview.chromium.org/6144005/diff/15001/src/parser.cc
File src/parser.cc (right):
http://codereview.chromium.org/6144005/diff/15001/src/parser.cc#newcode340
src/parser.cc:340: // TODO(mmaly): How to allocate temporary map and
free it when not needed?
I am trying to allocate the hash map only when function has >N
parameters and ideally free it as soon as parameter processing is done
(i.e. before we even hit the destructor). Is there a heap to use for
such transient memory? the new+delete didn't work and C++ complained
that object not allocated is being freed.
http://codereview.chromium.org/6144005/diff/15001/src/parser.cc#newcode351
src/parser.cc:351:
Moved the code from Scope to TemporaryScope since here is where I am
hoping to allocate and store the transient hash map.
http://codereview.chromium.org/6144005/diff/15001/src/parser.cc#newcode3324
src/parser.cc:3324:
This code runs even when not processing strict mode code, similarly for
the eval_loc and dupe_loc which take up stack space either way. With
slightly more complicated logic I could bring it down to just 1
Scanner::Location and a bool (because errors are reported in sequence
and I therefore only need to remember location of the error that will be
reported first) but am not sure how aggressively I should optimize.
http://codereview.chromium.org/6144005/diff/15001/src/parser.cc#newcode3409
src/parser.cc:3409: int position = function_token_position !=
RelocInfo::kNoPosition
This is still necessary because property get/set goes through here with
RelocInfo::kNoPosition. For now leaving as is but would like to find
better solution, once I get the feel for the right balance between perf
and good errors.
http://codereview.chromium.org/6144005/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
