[webkit-reviews] review denied: [Bug 95940] Make ref-count done on the function side of String::number() : [Attachment 162432] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Patrick R. Gansterer
<paroga at paroga.com>'s request for review:
Bug 95940: Make ref-count done on the function side of String::number()
https://bugs.webkit.org/show_bug.cgi?id=95940

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=162432&action=review


The template generalization taking StringType is not very useful.

This remind me we should have StringBuilder::append(integer). Since you are
creating generalizations, could you find a template that could do:
1) String initialization like today
2) If used in StringBuilder, call StringBuilder::append(LChar*, length) on buf.


> Source/JavaScriptCore/ChangeLog:10
> +	   instead of being on the caller side. Keep the conversion code in its
own
> +	   file so we can reuse the function for AtomicString::number() later
too.

I am pretty sure we don't need AtomicString::number().
But I actually like your idea of keeping that code separated. It will keep
WTFString.cpp cleaner.

> Source/WTF/ChangeLog:20
> +	   (WTF):

Not useful.

> Source/WTF/ChangeLog:25
> +	   (WTF):

Ditto.

> Source/WTF/wtf/text/IntegerToStringConversion.h:46
> +template<typename StringType, typename UnsignedIntegerType, bool Negative>
> +static StringType numberToStringImpl(UnsignedIntegerType number)

I am not a fan of the template parameter for negative numbers.

I understand it is shorter, but do you thing it is clearer?

I am not against it either.

> Source/WTF/wtf/text/WTFString.h:228
> +    WTF_EXPORT_STRING_API static String number(short);

The version taking "short" was not there on purpose.

I don't remember why I left unsigned short though... Might be related to the
KURL works.

> Source/WebKit2/ChangeLog:10
> +	   Un-Inline String::number() to make the ref-count done on the
function side
> +	   instead of being on the caller side. Keep the conversion code in its
own
> +	   file so we can reuse the function for AtomicString::number() later
too.

No need to copy the description.

> Tools/ChangeLog:10
> +	   Un-Inline String::number() to make the ref-count done on the
function side
> +	   instead of being on the caller side. Keep the conversion code in its
own
> +	   file so we can reuse the function for AtomicString::number() later
too.

No need to copy this for Tools.