[webkit-reviews] review denied: [Bug 41920] Avoid slow-path for put() in Array.splice() : [Attachment 61290] Proposed patch v3

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Andreas Kling
<andreas.kling at nokia.com>'s request for review:
Bug 41920: Avoid slow-path for put() in Array.splice()
https://bugs.webkit.org/show_bug.cgi?id=41920

Attachment 61290: Proposed patch v3
https://bugs.webkit.org/attachment.cgi?id=61290&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    if (creationMode == CreateInitialized) {
> +	   JSValue* vector = m_storage->m_vector;
> +	   for (size_t i = 0; i < initialCapacity; ++i)
> +	       vector[i] = JSValue();
> +	   m_storage->m_numValuesInVector = 0;
> +    } else
> +	   m_storage->m_numValuesInVector = initialCapacity;

It's not safe to leave the vector uninitialized. Garbage collection could
happen if one of the calls to getProperty does any object allocation, which can
definitely happen in many ways, including getters and DOM objects. And if
garbage collection does happen, we will need to survive a call to the
markChildren function.

> +	   enum ArrayCreationMode { CreateCompactUninitialized,
CreateInitialized };

While it is nice to scope the enum to the class, it makes the call sites too
ugly. I think it's fine to define this at namespace scope instead.

> +	   void uncheckedSetIndex(unsigned i, JSValue v)
> +	   {
> +	       m_storage->m_vector[i] = v;
> +	   }

You could put assertions in here to make it clearer what those things are that
are not checked.