[webkit-reviews] review granted: [Bug 197228] TypeArrays should not store properties that are canonical numeric indices : [Attachment 368404] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Tadeu Zagallo <tzagallo at apple.com>'s
request for review:
Bug 197228: TypeArrays should not store properties that are canonical numeric
indices
https://bugs.webkit.org/show_bug.cgi?id=197228

Attachment 368404: Patch

https://bugs.webkit.org/attachment.cgi?id=368404&action=review




--- Comment #22 from Darin Adler <darin at apple.com> ---
Comment on attachment 368404
  --> https://bugs.webkit.org/attachment.cgi?id=368404
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=368404&action=review

I am still a bit confused about what the standard calls for. I would add these
test cases too:

    '4294967294' // maximum array index
    '4294967295' // maximum array index + 1
    '4294967296' // maximum array index + 2

I understand the desire to not confuse array indices with other property keys,
but I don’t get how the people who wrote the standard chose to divide things. I
would have thought they’d make it illegal to have other properties if the keys
are valid array indices, but instead they defined a seemingly arbitrary
superset of valid array indices.

> Source/JavaScriptCore/runtime/JSTypedArrays.h:49
> +JS_EXPORT_PRIVATE Optional<double> canonicalNumericIndexString(const
PropertyName&);
> +
> +#if !ASSERT_DISABLED
> +JS_EXPORT_PRIVATE bool isValidIndex(double);
> +#endif

I think it’s OK to land it like this, but I think it would be cleaner to
include only a function that returns a boolean:

    bool isCanonicalNumericIndeString(const PropertyName&);

We can put the consistency checks and assertions inside the function. Also not
entirely sure this function belongs in JSTypedArrays.h. I know that typed array
implementations need to do this check, but the check is a more basic language
property.