Bug 178062 – [JSC] Use fastJoin in Array#toString

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Use fastJoin in Array#toString

[Yusuke Suzuki]

Created attachment 323124 [details]
Patch

[Yusuke Suzuki]

Created attachment 323125 [details]
Patch

[Darin Adler]

Comment on attachment 323125 [details]
Patch

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

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:672
> +        if (element.isUndefinedOrNull())
> +            continue;

Why does the ENABLE(INTL) version put in extra commas, but the !ENABLE(INTL) version doesn’t? There is no stringJoiner.appendEmptyString() here. Does your patch change behavior in one of the two cases? Do we have test coverage?

Can we put ENABLE(INTL) around less of the function? It seems like the code is *almost* identical. There are only three differences.

> Source/JavaScriptCore/runtime/JSStringJoiner.h:41
> +    void appendInt32(VM&, int32_t);
> +    void appendDouble(VM&, double);

I probably would have used overloading and named these both appendNumber or appendNumeral. I believe that if someone used it with any other type besides int32_t and double, it would not compile because the overloading would be ambiguous. That’s how the underlying numericStrings.add works.

But your way is clear and explicit so I am OK with it.

[Darin Adler]

Comment on attachment 323125 [details]
Patch

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

>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:672
>> +            continue;
> 
> Why does the ENABLE(INTL) version put in extra commas, but the !ENABLE(INTL) version doesn’t? There is no stringJoiner.appendEmptyString() here. Does your patch change behavior in one of the two cases? Do we have test coverage?
> 
> Can we put ENABLE(INTL) around less of the function? It seems like the code is *almost* identical. There are only three differences.

On reflection, obviously the patch doesn’t change anything because this is toLocaleString, not toString, but still this is strange.

[Yusuke Suzuki]

Comment on attachment 323125 [details]
Patch

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

Thank you!

>>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:672
>>> +            continue;
>> 
>> Why does the ENABLE(INTL) version put in extra commas, but the !ENABLE(INTL) version doesn’t? There is no stringJoiner.appendEmptyString() here. Does your patch change behavior in one of the two cases? Do we have test coverage?
>> 
>> Can we put ENABLE(INTL) around less of the function? It seems like the code is *almost* identical. There are only three differences.
> 
> On reflection, obviously the patch doesn’t change anything because this is toLocaleString, not toString, but still this is strange.

Yeah, this patch does not change toLocaleString. But right, it looks incorrect.
According to the spec[1], we should take ENABLE(INTL) version's behavior.
I'll clean up this part and add a test.
[1]: https://tc39.github.io/ecma402/#sup-array.prototype.tolocalestring

>> Source/JavaScriptCore/runtime/JSStringJoiner.h:41
>> +    void appendDouble(VM&, double);
> 
> I probably would have used overloading and named these both appendNumber or appendNumeral. I believe that if someone used it with any other type besides int32_t and double, it would not compile because the overloading would be ambiguous. That’s how the underlying numericStrings.add works.
> 
> But your way is clear and explicit so I am OK with it.

OK, I'll use overloading version since the underlying numericStrings.add follows this. I'll change them to `appendNumber`.

[Yusuke Suzuki]

Committed r223834: <https://trac.webkit.org/changeset/223834>

[Radar WebKit Bug Importer]

<rdar://problem/35568901>