[webkit-reviews] review denied: [Bug 32012] JavaScript delete operator returns true for string properties : [Attachment 44159] Revised patch with more test cases and using toArrayIndex() when converting identifier to string index

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Kent Hansen <kent.hansen at nokia.com>'s
request for review:
Bug 32012: JavaScript delete operator returns true for string properties
https://bugs.webkit.org/show_bug.cgi?id=32012

Attachment 44159: Revised patch with more test cases and using toArrayIndex()
when converting identifier to string index
https://bugs.webkit.org/attachment.cgi?id=44159&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   unsigned toIndex(const Identifier& propertyName, bool *ok);

WebKit coding style requires putting the "*" next to the "bool", not the "ok".

> +    inline unsigned JSString::toIndex(const Identifier& propertyName, bool
*ok)
> +    {
> +	   unsigned i = propertyName.toArrayIndex(ok);
> +	   *ok = *ok && canGetIndex(i);
> +	   return i;
> +    }

I think it's slightly strange to include the canGetIndex call inside a function
named toIndex. That's inconsistent with other functions named toIndex, and so
perhaps the function should have a different name. But since I don't have
another to suggest, we can probably live with this. I would probably call the
output variable something more like isIndex rather than ok given the slightly
broader purpose of the function.

> +    (void)internalValue()->toIndex(propertyName, &isIndex);

WebKit code does not use this style, with explicit casts to (void) when
ignoring function results.

> +description("This page tests deletion of properties on a string object.");
> +
> +var str = "abc";
> +shouldBe('str.length', '3');
> +shouldBe('delete str.length', 'false');
> +shouldBe('delete str[0]', 'false');
> +shouldBe('delete str[1]', 'false');
> +shouldBe('delete str[2]', 'false');
> +shouldBe('delete str[3]', 'true');
> +shouldBe('delete str[-1]', 'true');
> +shouldBe('delete str[4294967294]', 'true');
> +shouldBe('delete str[4294967295]', 'true');
> +shouldBe('delete str.foo', 'true');

Looking at the test and test results I now realize that there is no detectable
behavior change from what I asked you to do in the last patch, since indices
that are beyond of the length of the string are treated no differently from
identifiers that are not valid indices. Thus the special value that is a UInt32
but not a valid index doesn't actually need special handling. That means that
using toStrictUInt32 was already acceptable and even slightly more efficient.
Using toIndex, while more logical, adds an unnecessary additional check for
that special value.

Ideally I would like to see a test that includes 4294967296 as well, and also
test cases for floating pointer numbers such as "0.0" and "0.1" to make sure
that "0.0" correctly deletes the zero element and "0.1" correctly does not.

I think the patch is fine as is, but I'm going to say review- because of the
two minor style issues that check-webkit-style will no doubt catch as well. You
could also consider my comments. Sorry for giving you conflicting advice.