<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 Number Format Functions"
href="https://bugs.webkit.org/show_bug.cgi?id=147605">bug 147605</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 #271263 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 Number Format Functions"
href="https://bugs.webkit.org/show_bug.cgi?id=147605#c10">Comment # 10</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [INTL] Implement Number Format Functions"
href="https://bugs.webkit.org/show_bug.cgi?id=147605">bug 147605</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=271263&action=diff" name="attach_271263" title="Patch">attachment 271263</a> <a href="attachment.cgi?id=271263&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=271263&action=review">https://bugs.webkit.org/attachment.cgi?id=271263&action=review</a>
<span class="quote">> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:70
> +IntlNumberFormat::~IntlNumberFormat()
> +{
> + if (m_numberFormat)
> + unum_close(m_numberFormat);
> +}</span >
I would have slightly preferred that we use std::unique_ptr instead.
struct UNumberFormatDeleter {
void operator()(UNumberFormat* numberFormat) const
{
if (numberFormat)
unum_close(numberFormat);
}
};
std::unique_ptr<UNumberFormat, UNumberFormatDeleter> m_numberFormat;
If we did that, we would not need to define ~IntlNumberFormat() and other code would be a bit simpler as well.
<span class="quote">> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:401
> + UNumberFormat* numberFormat = unum_open(style, nullptr, 0, m_locale.utf8().data(), nullptr, &status);</span >
If we used std::unique_ptr this would be:
auto numberFormat = std::unique_ptr<UNumberFormat, UNumberFormatDeleter>(unum_open(style, nullptr, 0, m_locale.utf8().data(), nullptr, &status));
<span class="quote">> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:406
> + unum_setTextAttribute(numberFormat, UNUM_CURRENCY_CODE, StringView(m_currency).upconvertedCharacters(), 3, &status);</span >
If we used std::unique_ptr, these lines would need to use get().
<span class="quote">> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:419
> + unum_close(numberFormat);</span >
We would not need this if we used std::unique_ptr.
<span class="quote">> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:423
> + m_numberFormat = numberFormat;</span >
This line would need to be:
m_numberFormat = WTFMove(numberFormat);
<span class="quote">> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:436
> + if (bitwise_cast<uint64_t>(number) == bitwise_cast<uint64_t>(-0.0))
> + number = 0.0;</span >
How is the bitwise_cast code better than:
// Map negative zero to positive zero.
if (!number)
number = 0;
I don’t think it is.
<span class="quote">> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:447
> + return state.vm().throwException(&state, createError(&state, ASCIILiteral("Failed to format a number.")));</span >
Where is the test coverage for this? When can this fail?</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>