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(); On 2012/07/06 10:53:22, Michael Starzinger wrote:
Use name->GetIsolate() instead.
Done. 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( On 2012/07/06 10:53:22, Michael Starzinger wrote:
Let's name that "MakeModuleExportAccessor()"
Hm, I'm following the naming convention of the existing accessors here. This one is a factory for a "ModuleExport" accessor, thus the name. (Note that the class is already named Accessors, so it's implied.) https://chromiumcodereview.appspot.com/10690043/diff/1/src/accessors.h#newcode90 src/accessors.h:90: Handle<String> property, int index, PropertyAttributes attributes); On 2012/07/06 10:53:22, Michael Starzinger wrote:
Rename "property" to "name" here.
Done. 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) On 2012/07/06 10:53:22, Michael Starzinger wrote:
Can we move that out into a separate group instead of interleaving it
with the
non-optimized nodes.
Done. https://chromiumcodereview.appspot.com/10690043/diff/1/src/ast.cc#newcode1119 src/ast.cc:1119: #undef DONT_SELFOPTIMIZE_NODE On 2012/07/06 10:53:22, Michael Starzinger wrote:
Also #undef DONT_CACHE_NODE here.
Done. 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) : On 2012/07/06 10:53:22, Michael Starzinger wrote:
The colon should be on the next line.
Done. https://chromiumcodereview.appspot.com/10690043/diff/1/src/ast.h#newcode598 src/ast.h:598: On 2012/07/06 10:53:22, Michael Starzinger wrote:
For consistency we should drop this empty newline.
Done. https://chromiumcodereview.appspot.com/10690043/diff/1/src/ast.h#newcode602 src/ast.h:602: explicit Module(Zone* zone) : On 2012/07/06 10:53:22, Michael Starzinger wrote:
The colon should be on the next line.
Done. https://chromiumcodereview.appspot.com/10690043/diff/1/src/ast.h#newcode605 src/ast.h:605: explicit Module(Interface* interface, Block* body = NULL) : On 2012/07/06 10:53:22, Michael Starzinger wrote:
The colon should be on the next line.
Done. 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); On 2012/07/06 10:53:22, Michael Starzinger wrote:
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.
Done. 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); On 2012/07/06 10:53:22, Michael Starzinger wrote:
I would prefer Smi::FromInt(0) instead of NULL here.
Done (also in old code below). 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"], On 2012/07/06 10:53:22, Michael Starzinger wrote:
Should we maybe name this "module_export_undefined"?
Done. 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. On 2012/07/06 10:53:22, Michael Starzinger wrote:
Empty newline between accessors.
Done. 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()) { On 2012/07/06 10:53:22, Michael Starzinger wrote:
Can we merge that into one condition?
Done. https://chromiumcodereview.appspot.com/10690043/diff/1/src/scopes.cc#newcode1377 src/scopes.cc:1377: USE(instance->PreventExtensions()); On 2012/07/06 10:53:22, Michael Starzinger wrote:
This might request a GC, so you should use
JSObject::PreventExtensions(instance)
here.
Done. 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. On 2012/07/06 10:53:22, Michael Starzinger wrote:
Drop one "may" and "s/requires/require".
Done. https://chromiumcodereview.appspot.com/10690043/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
