[webkit-reviews] review denied: [Bug 128864] Vector with inline capacity should work with non-PODs : [Attachment 224281] the patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Filip Pizlo
<fpizlo at apple.com>'s request for review:
Bug 128864: Vector with inline capacity should work with non-PODs
https://bugs.webkit.org/show_bug.cgi?id=128864

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=224281&action=review


I would reeaaaallly like a test, even only on OS X. That will take 5 minutes
and will avoid any stupid mistakes like this in future refactoring.

> Source/WTF/wtf/Vector.h:480
> -	       std::swap(m_inlineBuffer, other.m_inlineBuffer);
> +	       swapInlineBuffer(other, mySize, otherSize);
>	       std::swap(m_capacity, other.m_capacity);
>	   } else if (other.buffer() == other.inlineBuffer()) {
>	       other.m_buffer = m_buffer;
>	       m_buffer = inlineBuffer();
> -	       std::swap(m_inlineBuffer, other.m_inlineBuffer);
> +	       swapInlineBuffer(other, mySize, otherSize);

I am not sure I like swapInlineBuffer() in those cases though.

On remove, we just call the destructor. So it is basically garbage in one or
the other of the inlineBuffer and we will swap valid values with garbage.

In these two cases, I would instead do a moveOperation from the buffer to the
inline_buffer, followed by a call to the destructors on the unused
inlineBuffer.

> Source/WTF/wtf/Vector.h:535
> +	   for (unsigned i = swapBound; i--;)
> +	       std::swap(left[i], right[i]);

Why do you go from swapBound to zero instead of zero to swapBound-1?

> Source/WTF/wtf/Vector.h:715
> +	   Base::swap(other, m_size, other.m_size);

m_size is on VectorBufferBase, not sure you need to pass it here.

We could even have Base::swap() do the m_size swap to make things clearer.