[Webkit-unassigned] [Bug 41238] RegExp performance slow on Dromaeo benchmark

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=41238


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #59974|review?                     |review-
               Flag|                            |




--- Comment #8 from Darin Adler <darin at apple.com>  2010-06-29 10:51:32 PST ---
(From update of attachment 59974)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.