[webkit-reviews] review granted: [Bug 178062] [JSC] Use fastJoin in Array#toString : [Attachment 323125] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Oct 8 00:10:11 PDT 2017


Darin Adler <darin at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 178062: [JSC] Use fastJoin in Array#toString
https://bugs.webkit.org/show_bug.cgi?id=178062

Attachment 323125: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 323125
  --> https://bugs.webkit.org/attachment.cgi?id=323125
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.


More information about the webkit-reviews mailing list