[webkit-reviews] review denied: [Bug 77018] implement Number.toLocaleString() using ICU : [Attachment 136705] Require libicuuc >= 4.6

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Rob Buis <rwlbuis at gmail.com>'s request
for review:
Bug 77018: implement Number.toLocaleString() using ICU
https://bugs.webkit.org/show_bug.cgi?id=77018

Attachment 136705: Require libicuuc >= 4.6
https://bugs.webkit.org/attachment.cgi?id=136705&action=review

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


> Source/JavaScriptCore/runtime/NumberPrototype.cpp:68
> +static UNumberFormat* cachedNumberFormatter = 0;

This global is used only inside the formatLocaleNumber function, so I suggest
moving it in there.

Or, better, we could put it in per-thread globals so we don’t have to do all
the locking.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:75
> +static JSCell* formatLocaleNumber(ExecState* exec, JSValue v)

Could we use a word, numberValue or value, instead of the letter “v” here
please?

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:81
> +    UNumberFormat* nf = 0;

Could we use a word, formatter, instead of the letters “nf” here please?

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:83
> +	   const char* cachedNumberFormatterLocale =
unum_getLocaleByType(cachedNumberFormatter, ULOC_REQUESTED_LOCALE, &status);

This looks inefficient. The value of caching a formatter is lost if we are
doing all this work every time.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:90
> +	       // locale changed... we'll need a new formatter

WebKit comments are typically formatted as sentences.

    // Locale changed, so we'll need a new formatter.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:110
> +	   int32_t num = v.asInt32();

Our coding style typically prefers a word, number, over the abbreviation “num”
for a local variable like this one.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:117
> +	   if (v.isDouble())
> +	       num = v.asDouble();
> +	   else
> +	       num = v.toNumber(exec);

Why the special case for v.isDouble? That’s already what toNumber would do, so
why add extra code.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:119
> +	   DecimalNumber d(num);

Please use a word here rather than the letter “d” for the name of this local
variable.

> Source/WTF/wtf/unicode/icu/UnicodeICU.cpp:36
> +#if OS(DARWIN) && USE(CF) && !PLATFORM(QT)
> +    // Mac OS X doesn't set UNIX locale to match user-selected one, so ICU
default doesn't work.
> +    // FIXME: Need a way to get the numeric locale on Darwin
> +    CRASH();
> +#else

If the ifdef was around the function implementation then we’d get a link time
failure instead of a runtime crash, which is much better. Please put the ifdef
around the entire function, not just in the body.