[Webkit-unassigned] [Bug 147602] [INTL] Implement Intl.NumberFormat.prototype.resolvedOptions ()

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=147602

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #267982|review?                     |review+
              Flags|                            |

--- Comment #24 from Darin Adler <darin at apple.com> ---
Comment on attachment 267982
  --> https://bugs.webkit.org/attachment.cgi?id=267982
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267982&action=review

Seems like a reasonable first cut. We need to remember to do some performance testing and tuning on this; there’s a lot of memory allocation here.

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:94
> +static bool isWellFormedCurrencyCode(const String& currency)

Might be worth marking some of these super-simple functions inline, especially if they have only one or two call sites.

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:100
> +static unsigned computeCurrencySortKey(const char* currency)

Ditto.

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:106
> +static unsigned extractCurrencySortKey(std::pair<const char*, unsigned>* currencyMinorUnit)

Ditto.

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:143
> +    std::array<std::pair<const char*, unsigned>, 26> currencyMinorUnits = { {
> +        { "BHD", 3 },
> +        { "BIF", 0 },
> +        { "BYR", 0 },
> +        { "CLF", 4 },
> +        { "CLP", 0 },
> +        { "DJF", 0 },
> +        { "GNF", 0 },
> +        { "IQD", 3 },
> +        { "ISK", 0 },
> +        { "JOD", 3 },
> +        { "JPY", 0 },
> +        { "KMF", 0 },
> +        { "KRW", 0 },
> +        { "KWD", 3 },
> +        { "LYD", 3 },
> +        { "OMR", 3 },
> +        { "PYG", 0 },
> +        { "RWF", 0 },
> +        { "TND", 3 },
> +        { "UGX", 0 },
> +        { "UYI", 0 },
> +        { "VND", 0 },
> +        { "VUV", 0 },
> +        { "XAF", 0 },
> +        { "XOF", 0 },
> +        { "XPF", 0 }
> +    } };

I believe this will do some work every time the function is called. We need to make sure this actually gets compiled in a way that doesn’t do the array initialization each time, for efficiently. Might need to eschew the use of std::array, or mark this as const, to achieve that.

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:144
> +    std::pair<const char*, unsigned>* currencyMinorUnit = tryBinarySearch<std::pair<const char*, unsigned>>(currencyMinorUnits, currencyMinorUnits.size(), computeCurrencySortKey(currency.utf8().data()), extractCurrencySortKey);

Would read better with the use of auto*. Unclear on why we need to explicitly pass the type in when invoking the tryBinarySearch function template; I’d expect it to work without specifying that explicitly.

Should overload computeCurrencySortKey to work on a const String& or StringView correctly; doing memory allocation just to convert the String to a CString, just to get the first three characters of the string, is not great.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160122/035128ee/attachment-0001.html>