[webkit-reviews] review denied: [Bug 41238] RegExp performance slow on Dromaeo benchmark : [Attachment 59974] Updated patch.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Michael Saboff <msaboff at apple.com>'s
request for review:
Bug 41238: RegExp performance slow on Dromaeo benchmark
https://bugs.webkit.org/show_bug.cgi?id=41238

Attachment 59974: Updated patch.
https://bugs.webkit.org/attachment.cgi?id=59974&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Looks great.

> +	   Refactored Vector<>::operator=() method into a separate copy()
method 
> +	   that it calls.  The copy() method is explicitly used for deep
copying.

I don't understand this change. Why can't we use the assignment operator
directly?

One small problem I see with adding a copy function is that in WebKit functions
with copy in their name are typically functions that return a copy of an object
or some sort of new object. So using the name copy for a function with no
return value that copies data into an object is a bit non-standard and thus
possibly confusing.

> +    m_lastMatchString = UString::null();
> +    m_lastMatchStart = -1;
> +    m_lastOVector.resize(0);

This site needs the UString() and shrink(0) changes we made earlier in the same
file.

>      template<typename T, size_t inlineCapacity>
>      Vector<T, inlineCapacity>& Vector<T, inlineCapacity>::operator=(const
Vector<T, inlineCapacity>& other)
>      {
> +	   if (&other != this)
> +	       copy(other);
> +	   
> +	   return *this;
> +    }

I think this should be marked inline. We don't want to add another level of
function call here.

> +    template<typename T, size_t inlineCapacity> template<size_t
otherCapacity>
> +    void Vector<T, inlineCapacity>::copy(const Vector<T, otherCapacity>&
other)
> +    {
>	   if (&other == this)
> -	       return *this;
> +	       return;

Both the copy function and the assignment operator are doing this check. That
seems like overkill to me unless there's a reason I'm missing this needs to be
done.

review- because I don't want to add this copy function unless there's a good
reason to do so, and the patch doesn't give a reason.