<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [INTL] Implement Intl.NumberFormat.prototype.resolvedOptions ()"
href="https://bugs.webkit.org/show_bug.cgi?id=147602#c27">Comment # 27</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [INTL] Implement Intl.NumberFormat.prototype.resolvedOptions ()"
href="https://bugs.webkit.org/show_bug.cgi?id=147602">bug 147602</a>
from <span class="vcard"><a class="email" href="mailto:sukolsak@gmail.com" title="Sukolsak Sakshuwong <sukolsak@gmail.com>"> <span class="fn">Sukolsak Sakshuwong</span></a>
</span></b>
<pre>Thanks.
(In reply to <a href="show_bug.cgi?id=147602#c24">comment #24</a>)
<span class="quote">> Comment on <span class="bz_obsolete"><a href="attachment.cgi?id=267982&action=diff" name="attach_267982" title="Patch">attachment 267982</a> <a href="attachment.cgi?id=267982&action=edit" title="Patch">[details]</a></span>
> Patch
>
> View in context:
> <a href="https://bugs.webkit.org/attachment.cgi?id=267982&action=review">https://bugs.webkit.org/attachment.cgi?id=267982&action=review</a>
>
> 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.</span >
Inlined.
<span class="quote">> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:100
> > +static unsigned computeCurrencySortKey(const char* currency)
>
> Ditto.</span >
Marked inline.
<span class="quote">> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:106
> > +static unsigned extractCurrencySortKey(std::pair<const char*, unsigned>* currencyMinorUnit)
>
> Ditto.</span >
I am not sure if it will make a difference, because we don't call this function here. We pass it to tryBinarySearch.
<span class="quote">> > 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.</span >
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".
<span class="quote">> > 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.</span >
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())
<span class="quote">> 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.</span >
Done.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>