[webkit-reviews] review denied: [Bug 38142] RegExp caching : [Attachment 54334] RegExp caching

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Renata Hodovan
<hodovan.renata at stud.u-szeged.hu>'s request for review:
Bug 38142: RegExp caching
https://bugs.webkit.org/show_bug.cgi?id=38142

Attachment 54334: RegExp caching
https://bugs.webkit.org/attachment.cgi?id=54334&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
I would have called "get" "lookupOrCreate" to match the Create pattern of the
rest of the code.

I assume there is no gotchas to making JSGlobalData larger?

+    RefPtr<RegExp> cacheRegExp;
should be cachedRegExp, not "cache"

I think it would be cleaner to move all the creation logic (under the if
(!cacheRegExp) block) into its own function.  Then the function reads:

if (!cachedRegExp)
     return createAndCache(flags, patternString);
return cacheRegExp.release()

Should be m_
+    , ptr(-1) 
+    , isFull(false) 

This code fails for regexps over maxCacheablePatternLength in length.  It
should early return creating a new one each time.

I would have made a countFlags(flags) method for:
+    if (flags.find('g') != UString::NotFound)
+	 flagsCnt += 4;
+    if (flags.find('i') != UString::NotFound)
+	 flagsCnt += 2;
+    if (flags.find('m') != UString::NotFound)
+	 flagsCnt += 1;

Also, countFlags probably goes on RegExpKeyType.

Why RegExpKeyType instead of just RegExpKey?

I *really* like this idea.  I just think the code needs some refinement.