Bug 86312 – Generalize the number to String conversion from UString

• Status: RESOLVED
• Resolution: LATER
• Component: WebKit Misc.

[Benjamin Poulain]

Currently WTFString and UString convert number to String differently.

The first step to clean that is to move the code to WTF and use it in both UString and String.

[Benjamin Poulain]

Created attachment 141596 [details]
Patch

[Benjamin Poulain]

This is just a first step.

In follow up I want to:
1) make templates for that code (and replace all the functions of WTFString)
2) move the conversion functions of NumberPrototype.cpp to the header

After that, depending on the benchmarks, I will either move the code to StringImpl.cpp or leave it in the header and use it in NumericStrings.h.

[Build Bot]

Comment on attachment 141596 [details]
Patch

Attachment 141596 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12679156

[Benjamin Poulain]

Created attachment 141598 [details]
Patch

[Darin Adler]

Comment on attachment 141598 [details]
Patch

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?

[Benjamin Poulain]

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

It is probably faster. The first intent was for cleaning but I can also try to find a place where that matters for performance.


> Do we need the explicit construction of the UString?

Nope, I just like explicit initialization better than implicit :)

> Need to explain that 3.
> [...]
> Can’t we make this case work just by using do/while instead of while? That seems better to me.
> [...]
> 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.

Here I just copied the methods from UString as-is to keep the review simple.

In a follow up, I want to modify those function to make it works like the one you reviewed for NumberPrototype.

If you want, I can rework this to do both change in one patch.

[Benjamin Poulain]

I will do the patch in an other bug. I have new improvements for both UString and WTF::String coming for this.