[webkit-reviews] review granted: [Bug 86312] Generalize the number to String conversion from UString : [Attachment 141598] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 86312: Generalize the number to String conversion from UString
https://bugs.webkit.org/show_bug.cgi?id=86312

Attachment 141598: Patch
https://bugs.webkit.org/attachment.cgi?id=141598&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=141598&action=review


Do performance testing results should a speed-up? I ask because the change log
does not seem to mention that.

I think we could improve a bit.

> Source/JavaScriptCore/runtime/UString.cpp:96
> +    return UString(numberToStringImpl(i));

Do we need the explicit construction of the UString?

> Source/WTF/wtf/text/IntegerToStringConversion.h:36
> +    LChar buf[1 + sizeof(i) * 3];

Need to explain that 3.

> Source/WTF/wtf/text/IntegerToStringConversion.h:41
> +    if (!i)
> +	   *--p = '0';

Can’t we make this case work just by using do/while instead of while? That
seems better to me.

> Source/WTF/wtf/text/IntegerToStringConversion.h:45
> +	   char minBuf[1 + sizeof(i) * 3];
> +	   snprintf(minBuf, sizeof(minBuf), "%d", INT_MIN);
> +	   return StringImpl::create(minBuf);

Since this is an integer constant, can’t we compile in a string constant? If we
can’t do any better, we could simply write it out, but I’d think we could do
something with a macro that stringifies and avoid allocating a buffer at all.

Or even better, just use a local variable of unsigned type and then we could
handle the minimum value along with all the rest of the values and avoid this
extra branch.

> Source/WTF/wtf/text/WTFString.cpp:428
> +    return String(numberToStringImpl(n));

Do we need the explicit construction of the String?