Nice!
(Tryjobs would be nice.)
Here are some comments (but not an exhaustive list, I'll have a closer look
at
some later point):
https://codereview.chromium.org/974213002/diff/1/src/background-parsing-task.cc
File src/background-parsing-task.cc (right):
https://codereview.chromium.org/974213002/diff/1/src/background-parsing-task.cc#newcode25
src/background-parsing-task.cc:25:
info->set_source_stream(source->source_stream.get());
Would it make sense to have a ctor of ParseInfo which takes these things
as params?
https://codereview.chromium.org/974213002/diff/1/src/background-parsing-task.cc#newcode34
src/background-parsing-task.cc:34: options =
ScriptCompiler::kNoCompileOptions;
Huh, the options & options_ modifying is a bit unclear and hard to see
that it's going correctly now.
https://codereview.chromium.org/974213002/diff/1/src/background-parsing-task.cc#newcode38
src/background-parsing-task.cc:38: options_ =
ScriptCompiler::kNoCompileOptions;
... why is this correct? Won't this just always kill the compile
options? (and we'll never get into the
source_->info->set_cached_data(&script_data); branch in Run).
Some tests *should* fail if you mess this up, though :)
https://codereview.chromium.org/974213002/diff/1/src/compiler.cc
File src/compiler.cc (right):
https://codereview.chromium.org/974213002/diff/1/src/compiler.cc#newcode171
src/compiler.cc:171: if (parse_info_)
parse_info_->DeleteAstValueFactory();
... I guess parse_info_ is not owned, CompilationInfo just happens to
point into it... so it's slightly surprising that the lifetime of
CompilationInfo affects what happens inside ParseInfo.
https://codereview.chromium.org/974213002/diff/1/src/compiler.h
File src/compiler.h (right):
https://codereview.chromium.org/974213002/diff/1/src/compiler.h#newcode381
src/compiler.h:381: ParseInfo* parse_info_;
Owned? Not owned? Who owns it?
https://codereview.chromium.org/974213002/diff/1/src/parser.h
File src/parser.h (right):
https://codereview.chromium.org/974213002/diff/1/src/parser.h#newcode27
src/parser.h:27: class ScriptData {
Can you put the added classes (ScriptData and ParseInfo) into a separate
file? parser.h is bloated enough already and diff-wise it's the same.
https://codereview.chromium.org/974213002/diff/1/src/parser.h#newcode228
src/parser.h:228: // TODO(titzer): these should *not* be part of
ParseInfo. Refactor Parser.
Yes! :)
https://codereview.chromium.org/974213002/diff/1/src/scopes.cc
File src/scopes.cc (right):
https://codereview.chromium.org/974213002/diff/1/src/scopes.cc#newcode12
src/scopes.cc:12: #include "src/scopes.h"
Nit, this should remain at the top...
https://codereview.chromium.org/974213002/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
