LGTM (with nits and comments). Just one real comment at the end of
scopes.cc,
the rest are mainly nits.
https://chromiumcodereview.appspot.com/10690043/diff/1/src/accessors.cc
File src/accessors.cc (right):
https://chromiumcodereview.appspot.com/10690043/diff/1/src/accessors.cc#newcode855
src/accessors.cc:855: Factory* factory = Isolate::Current()->factory();
Use name->GetIsolate() instead.
https://chromiumcodereview.appspot.com/10690043/diff/1/src/accessors.h
File src/accessors.h (right):
https://chromiumcodereview.appspot.com/10690043/diff/1/src/accessors.h#newcode89
src/accessors.h:89: static Handle<AccessorInfo> MakeModuleExport(
Let's name that "MakeModuleExportAccessor()"
https://chromiumcodereview.appspot.com/10690043/diff/1/src/accessors.h#newcode90
src/accessors.h:90: Handle<String> property, int index,
PropertyAttributes attributes);
Rename "property" to "name" here.
https://chromiumcodereview.appspot.com/10690043/diff/1/src/ast.cc
File src/ast.cc (right):
https://chromiumcodereview.appspot.com/10690043/diff/1/src/ast.cc#newcode1083
src/ast.cc:1083: DONT_CACHE_NODE(ModuleLiteral)
Can we move that out into a separate group instead of interleaving it
with the non-optimized nodes.
https://chromiumcodereview.appspot.com/10690043/diff/1/src/ast.cc#newcode1119
src/ast.cc:1119: #undef DONT_SELFOPTIMIZE_NODE
Also #undef DONT_CACHE_NODE here.
https://chromiumcodereview.appspot.com/10690043/diff/1/src/ast.h
File src/ast.h (right):
https://chromiumcodereview.appspot.com/10690043/diff/1/src/ast.h#newcode590
src/ast.h:590: ExportDeclaration(VariableProxy* proxy, Scope* scope) :
The colon should be on the next line.
https://chromiumcodereview.appspot.com/10690043/diff/1/src/ast.h#newcode598
src/ast.h:598:
For consistency we should drop this empty newline.
https://chromiumcodereview.appspot.com/10690043/diff/1/src/ast.h#newcode602
src/ast.h:602: explicit Module(Zone* zone) :
The colon should be on the next line.
https://chromiumcodereview.appspot.com/10690043/diff/1/src/ast.h#newcode605
src/ast.h:605: explicit Module(Interface* interface, Block* body = NULL)
:
The colon should be on the next line.
https://chromiumcodereview.appspot.com/10690043/diff/1/src/contexts.h
File src/contexts.h (right):
https://chromiumcodereview.appspot.com/10690043/diff/1/src/contexts.h#newcode434
src/contexts.h:434: bool IsBootstrappingOrValidContext(Object* object);
Can we make these two debug methods static again and pass in both
contexts as arguments. Because currently it's not intuitive what the
difference between "this" and "object" actually is.
https://chromiumcodereview.appspot.com/10690043/diff/1/src/heap.cc
File src/heap.cc (right):
https://chromiumcodereview.appspot.com/10690043/diff/1/src/heap.cc#newcode4928
src/heap.cc:4928: context->set_previous(NULL);
I would prefer Smi::FromInt(0) instead of NULL here.
https://chromiumcodereview.appspot.com/10690043/diff/1/src/messages.js
File src/messages.js (right):
https://chromiumcodereview.appspot.com/10690043/diff/1/src/messages.js#newcode259
src/messages.js:259: "export_undefined", ["Export '", "%0",
"' is not defined in module"],
Should we maybe name this "module_export_undefined"?
https://chromiumcodereview.appspot.com/10690043/diff/1/src/objects.h
File src/objects.h (right):
https://chromiumcodereview.appspot.com/10690043/diff/1/src/objects.h#newcode5861
src/objects.h:5861: // [scope_info]: Scope info.
Empty newline between accessors.
https://chromiumcodereview.appspot.com/10690043/diff/1/src/scopes.cc
File src/scopes.cc (right):
https://chromiumcodereview.appspot.com/10690043/diff/1/src/scopes.cc#newcode646
src/scopes.cc:646: if (is_global_scope() || is_module_scope()) {
Can we merge that into one condition?
https://chromiumcodereview.appspot.com/10690043/diff/1/src/scopes.cc#newcode1377
src/scopes.cc:1377: USE(instance->PreventExtensions());
This might request a GC, so you should use
JSObject::PreventExtensions(instance) here.
https://chromiumcodereview.appspot.com/10690043/diff/1/src/scopes.h
File src/scopes.h (right):
https://chromiumcodereview.appspot.com/10690043/diff/1/src/scopes.h#newcode599
src/scopes.h:599: // may potentially requires (forward) references to
others.
Drop one "may" and "s/requires/require".
https://chromiumcodereview.appspot.com/10690043/diff/1/test/mjsunit/harmony/module-recompile.js
File test/mjsunit/harmony/module-recompile.js (right):
https://chromiumcodereview.appspot.com/10690043/diff/1/test/mjsunit/harmony/module-recompile.js#newcode81
test/mjsunit/harmony/module-recompile.js:81: if (i > N)
print("impossible");
I am not sure if this will force recompilation. Unless you made sure
that top-level code was deoptimized after the last optimization loop.
https://chromiumcodereview.appspot.com/10690043/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
