LGTM with some comments:
http://codereview.chromium.org/8104/diff/1/2 File src/compilation-cache.cc (right): http://codereview.chromium.org/8104/diff/1/2#newcode35 Line 35: NUMBER_OF_ENTRY_KINDS = CompilationCache::_LAST_ENTRY LAST_ENTRY + 1 http://codereview.chromium.org/8104/diff/1/2#newcode147 Line 147: Handle<CompilationCacheTable> table = GetTable(REGEXP); Shouldn't this be tracking compilation cache misses and hits like the other lookup functions? http://codereview.chromium.org/8104/diff/1/2#newcode147 Line 147: Handle<CompilationCacheTable> table = GetTable(REGEXP); Be careful not to leak the table handle into the surrounding handle scope. You should have a HandleScope here like in the generic static Lookup function. http://codereview.chromium.org/8104/diff/1/2#newcode148 Line 148: return Handle<Object>(table->LookupRegExp(*source, flags)); The return value is different from the other lookup functions (they return empty handles on miss). Is there a good reason for this? http://codereview.chromium.org/8104/diff/1/3 File src/compilation-cache.h (right): http://codereview.chromium.org/8104/diff/1/3#newcode47 Line 47: _LAST_ENTRY I would change _LAST_ENTRY to LAST_ENTRY and let it be equal to REGEXP: enum Entry { ..., LAST_ENTRY = REGEXP }. The LAST_ENTRY should be the last - not an additional one. Then you also avoid dealing with it in a switch... http://codereview.chromium.org/8104/diff/1/3#newcode64 Line 64: static Handle<Object> LookupRegExp(Handle<String> source, Maybe you should explain return value in case of a cache miss? http://codereview.chromium.org/8104/diff/1/3#newcode67 Line 67: static void PutRegExp(Handle<String> source, Maybe you should rename the Associate method to PutFunction to make it consistent with PutRegExp? Maybe you should have a comment here that explains what happens with existing mappings? http://codereview.chromium.org/8104/diff/1/5 File src/factory.h (right): http://codereview.chromium.org/8104/diff/1/5#newcode311 Line 311: static void SetRegExpData(Handle<JSRegExp> regexp, Maybe a comment here? Does it set the type, source, flags, and data on the given regexp? I guess so. http://codereview.chromium.org/8104/diff/1/6 File src/jsregexp.cc (right): http://codereview.chromium.org/8104/diff/1/6#newcode148 Line 148: int flags = JSRegExp::NONE; Would it make sense to have some sort of opaque data type for RegExp flags? Using 'int' seems a bit too generic. Sometimes we use an enum with no entries; see Code::Flags. http://codereview.chromium.org/8104/diff/1/6#newcode174 Line 174: bool in_cache = cached->IsFixedArray(); I would move the IsFixedArray logic into the cache to make it more consistent with the other CompilationCache lookup functions and change the check here to an empty handle check. Then you could also change the return type of LookupRegExp to Handle<FixedArray>. http://codereview.chromium.org/8104/diff/1/8 File src/log.cc (right): http://codereview.chromium.org/8104/diff/1/8#newcode381 Line 381: case JSRegExp::ATOM: Fix switch indentation. The cases should be indented with two spaces. http://codereview.chromium.org/8104/diff/1/10 File src/objects-debug.cc (right): http://codereview.chromium.org/8104/diff/1/10#newcode664 Line 664: case JSRegExp::ATOM: { Indent switch cases by two spaces. http://codereview.chromium.org/8104/diff/1/11 File src/objects-inl.h (right): http://codereview.chromium.org/8104/diff/1/11#newcode2180 Line 2180: return FixedArray::cast(data())->get(index); Maybe you should add an ASSERT that data isn't undefined or that the TypeTag() returns something not NOT_COMPILED. I know the FixedArray::cast will give you the same, but is seems more direct. http://codereview.chromium.org/8104/diff/1/12 File src/objects.cc (right): http://codereview.chromium.org/8104/diff/1/12#newcode5554 Line 5554: // RegExpKey carries a regexp object as key. Is this really so? It seems to hold a string and some flags? http://codereview.chromium.org/8104/diff/1/12#newcode5578 Line 5578: static uint32_t RegExpObjHash(Object* obj) { I don't like the Obj abbreviation here. RegExpObjectHash? http://codereview.chromium.org/8104/diff/1/13 File src/objects.h (right): http://codereview.chromium.org/8104/diff/1/13#newcode1862 Line 1862: Object* LookupRegExp(String* source, int flags); source -> src for consistency... or change src -> source. http://codereview.chromium.org/8104 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
