Replying outside rietveld since it's down. Fixed all comments unless otherwise noted.
> 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? I've added two new counters to track this. > 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? Nope, and I've changed it. I did actually wonder why the other functions didn't just return empty handles but it turns out that, as usual, I misunderstood handle.is_null() as checking for a non-empty handle containing the null value rather than checking for an empty handle. > 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. The reason I didn't do it was that I dislike casting back and forth between ints and enums. But it does help readability a lot so I've changed it. > 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. Good point. That was my eclipse code style setting showing through. --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
