[webkit-reviews] review denied: [Bug 32367] Add support for short Ropes (up to 3 entries) inline within JSString. : [Attachment 44601] The patch

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied Gavin Barraclough
<barraclough at apple.com>'s request for review:
Bug 32367: Add support for short Ropes (up to 3 entries) inline within
JSString.
https://bugs.webkit.org/show_bug.cgi?id=32367

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

------- Additional Comments from Oliver Hunt <oliver at apple.com>

> +    ALWAYS_INLINE JSValue jsString(ExecState* exec, Register* strings,
unsigned count)
> +    {
> +	   JSGlobalData* globalData = &exec->globalData();
> +	   ASSERT(count >= 3);
> +
> +	   unsigned ropeLength = 0;
> +	   for (unsigned i = 0; i < count; ++i) {
> +	       JSValue v = strings[i].jsValue();
> +	       if (LIKELY(v.isString())) {
> +		   ropeLength += asString(v)->ropeLength();
> +	       } else
> +		   ++ropeLength;
> +	   }
> +
> +	   if (ropeLength == 3)
> +	       return new (globalData) JSString(exec, strings[0].jsValue(),
strings[1].jsValue(), strings[2].jsValue());

This seems wrong
if i have a two arguments, and one says its ropeLength is 2, then ropeLength
will be 3, and we will take this code path which assumes each rope portion is
passed on the register file

I think maybe
a = "foo"
a += "bar"
c = "foo" + a
Will trigger this code path?