http://codereview.chromium.org/9615009/diff/1/src/ast.h File src/ast.h (right):
http://codereview.chromium.org/9615009/diff/1/src/ast.h#newcode614 src/ast.h:614: explicit ModuleLiteral(Block* body, Interface* interface) Remove "explicit". http://codereview.chromium.org/9615009/diff/1/src/ast.h#newcode2519 src/ast.h:2519: // Out-of-line inline constructors. I guess this here for cyclic dependencies reasons, so a remark in the comment would be nice. http://codereview.chromium.org/9615009/diff/1/src/interface.cc File src/interface.cc (right): http://codereview.chromium.org/9615009/diff/1/src/interface.cc#newcode48 src/interface.cc:48: ZoneHashMap::Entry* p = map->Lookup(name.location(), name->Hash(), false); Hash relies on the actual String contents, while Match relies on pointer identity. Therefore Hash is a bit of overkill, you could hash the pointer value. But I am not sure how performance-critical this is, so I leave this up to you. http://codereview.chromium.org/9615009/diff/1/src/interface.cc#newcode169 src/interface.cc:169: this->DoAdd(p->key, p->hash, static_cast<Interface*>(p->value), ok); If I see this correctly, this recursion is potentially bounded only by the structure of the input. Although this won't be a problem in practice, I think we will have to add a guard against pathological cases (or attacks). http://codereview.chromium.org/9615009/diff/1/src/interface.h File src/interface.h (right): http://codereview.chromium.org/9615009/diff/1/src/interface.h#newcode31 src/interface.h:31: #include "zone-inl.h" Do we really need a zone-inl.h here or is zone.h enough? Furthermore, a lot of entities are used in the current header which get only accidentally pulled in by zone-inl.h, so a bit more IWYU would be nice, making things less fragile. http://codereview.chromium.org/9615009/diff/1/src/interface.h#newcode40 src/interface.h:40: // exports ::= {name : interface, [CLOSED]} Hmmm, according to the code below, VALUEs can be CLOSED, too. Either the comment or the code is wrong... http://codereview.chromium.org/9615009/diff/1/src/interface.h#newcode52 src/interface.h:52: static Interface value_interface(VALUE); Huh? Why is this a singleton? http://codereview.chromium.org/9615009/diff/1/src/interface.h#newcode122 src/interface.h:122: enum Flags { // All flags are monotonous Hmmm, I am not totally happy with that representation. I think what is actually modeled is something like: data Kind = Value | Module; data Flexibility = Flexible | Frozen data State = Unknown | Known Kind Flexibility This type has 5 values, your type has 8. It would be nicer if the C stuff would be closer to the Haskell types above, but I leave it up to you to polish this or not. http://codereview.chromium.org/9615009/diff/1/src/scopes.cc File src/scopes.cc (right): http://codereview.chromium.org/9615009/diff/1/src/scopes.cc#newcode1033 src/scopes.cc:1033: if (!inner_scopes_[i]->ResolveVariablesRecursively(info, factory)) I know that this recursive call has been there before, but shouldn't we guard against pathological cases/attacks here? http://codereview.chromium.org/9615009/diff/1/test/mjsunit/harmony/module-parsing.js File test/mjsunit/harmony/module-parsing.js (right): http://codereview.chromium.org/9615009/diff/1/test/mjsunit/harmony/module-parsing.js#newcode30 test/mjsunit/harmony/module-parsing.js:30: // Test basic module syntax, with and without ASI. What's "ASI"? Asociación Salvadoreña de Industriales? :-) http://codereview.chromium.org/9615009/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
