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

[bugzilla-daemon at webkit.org]

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

--- Comment #27 from Sukolsak Sakshuwong <sukolsak at gmail.com> ---
Thanks.

(In reply to comment #24)
> Comment on attachment 267982 [details]
> 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.

Inlined.

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

Marked inline.

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

I am not sure if it will make a difference, because we don't call this function here. We pass it to tryBinarySearch.

> > 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.

Changed it to std::pair<const char*, unsigned>[]. I tried using std::initializer_list but got an error saying that it "does not provide a subscript operator".

> > 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.

Used auto*. It looks like we have to explicitly pass the type. Otherwise, I get this error:

/WebKit/Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:145:31: error: no matching function for call to 'tryBinarySearch'
    auto* currencyMinorUnit = tryBinarySearch(currencyMinorUnits, WTF_ARRAY_LENGTH(currencyMinorUnits), computeCurrencySortKey(StringView(currency)), extractCurrencySortKey);
                              ^~~~~~~~~~~~~~~

...

/WebKit/WebKitBuild/Debug/usr/local/include/wtf/StdLibExtras.h:240:26: note: candidate template ignored: couldn't infer template argument 'ArrayElementType'
inline ArrayElementType* tryBinarySearch(ArrayType& array, size_t size, KeyType key, ExtractKey extractKey = ExtractKey())

> 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.

Done.

-- 
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/20160203/8272e298/attachment-0001.html>