[webkit-reviews] review granted: [Bug 76079] v8-regexp spends 35% of its time allocating and copying internal regexp results data : [Attachment 122052] Patch for review

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Michael Saboff
<msaboff at apple.com>'s request for review:
Bug 76079: v8-regexp spends 35% of its time allocating and copying internal
regexp results data
https://bugs.webkit.org/show_bug.cgi?id=76079

Attachment 122052: Patch for review
https://bugs.webkit.org/attachment.cgi?id=122052&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=122052&action=review


r=me

> Source/JavaScriptCore/runtime/RegExpConstructor.cpp:100
> +RegExpResult& RegExpResult::operator=(const RegExpConstructorPrivate& rhs)

Should this be inlined?

> Source/JavaScriptCore/runtime/RegExpConstructor.cpp:143
>  RegExpMatchesArray::~RegExpMatchesArray()

You can remove this entirely -- that will make Mark's work a little easier.

> Source/JavaScriptCore/runtime/RegExpMatchesArray.h:120
> +	   RegExpResult privateData;

Minor nit: I would call this variable "regExpResult". The class can have an
arbitrary amount of private data -- what's unique about this data is that it's
a regular expression result.

> Source/JavaScriptCore/runtime/RegExpMatchesArray.h:121
> +	   bool m_filledArrayInstance;

Minor nit: I prefer the style of prefacing boolean predicates with "did" --
"m_didFillArrayInstance". Otherwise, it's ambiguous whether
"filledArrayInstance" is a boolean predicate or a noun.