<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span> changed
<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>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #267982 Flags</td>
<td>review?
</td>
<td>review+
</td>
</tr></table>
<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#c24">Comment # 24</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:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span></b>
<pre>Comment on <span class=""><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.
<span class="quote">> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:94
> +static bool isWellFormedCurrencyCode(const String& currency)</span >
Might be worth marking some of these super-simple functions inline, especially if they have only one or two call sites.
<span class="quote">> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:100
> +static unsigned computeCurrencySortKey(const char* currency)</span >
Ditto.
<span class="quote">> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:106
> +static unsigned extractCurrencySortKey(std::pair<const char*, unsigned>* currencyMinorUnit)</span >
Ditto.
<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 }
> + } };</span >
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 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);</span >
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.</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>