<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 #270575 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#c31">Comment # 31</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=270575&action=diff" name="attach_270575" title="Patch">attachment 270575</a> <a href="attachment.cgi?id=270575&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=270575&action=review">https://bugs.webkit.org/attachment.cgi?id=270575&action=review</a>
<span class="quote">> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:3
> + * Copyright (C) 2015 Sukolsak Sakshuwong (<a href="mailto:sukolsak@gmail.com">sukolsak@gmail.com</a>)</span >
This year is 2016, not 2015.
<span class="quote">> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:96
> + ASSERT(currency.length() == 3 && currency.isAllSpecialCharacters<isASCIIAlpha>());</span >
This assertion needs to be isASCIIUpper, not just isASCIIAlpha.
Normally we don’t use && in assertions. Instead we write multiple assertions. That way if one clause fires, we know which one it was. In this case I think 4 separate assertions would be ideal so we know which one failed. 2 would be OK, though.
<span class="quote">> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:103
> + ASSERT(strlen(currency) == 3 && isAllSpecialCharacters<isASCIIAlpha>(currency, 3));
> + return (currency[0] << 16) + (currency[1] << 8) + currency[2];</span >
Ditto.
Could also just make computeCurrencySortKey a function template instead of writing it twice, except for the length check in the assertion, but there are tricks for that too. For example, StringView(currency).length() should work with both const String& and const char*.
Another option would be to use a single function that takes a StringView instead of two overloads.
<span class="quote">> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:194
> + m_locale = result.get(ASCIILiteral("locale"));</span >
Since we are going to make an Identifier here, not a String, ASCIILiteral is not idea. Might ask Ben Poulain or some other JavaScriptCore experts what the preferred style is for this kind of thing.
<span class="quote">> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:197
> + m_numberingSystem = result.get(ASCIILiteral("nu"));</span >
Ditto.
<span class="quote">> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:240
> + currency = currency.upper();</span >
This should be convertToASCIIUppercase, not upper, for better efficiency, and because I am working hard to eliminate upper entirely except for possibly one or two call sites.</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>