[Webkit-unassigned] [Bug 147605] [INTL] Implement Number Format Functions

[bugzilla-daemon at webkit.org]

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

--- Comment #13 from Sukolsak Sakshuwong <sukolsak at gmail.com> ---
(In reply to comment #10)
> Comment on attachment 271263 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271263&action=review
> 
> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:70
> > +IntlNumberFormat::~IntlNumberFormat()
> > +{
> > +    if (m_numberFormat)
> > +        unum_close(m_numberFormat);
> > +}
> 
> 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.
> 
> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:401
> > +    UNumberFormat* numberFormat = unum_open(style, nullptr, 0, m_locale.utf8().data(), nullptr, &status);
> 
> 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));
> 
> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:406
> > +        unum_setTextAttribute(numberFormat, UNUM_CURRENCY_CODE, StringView(m_currency).upconvertedCharacters(), 3, &status);
> 
> If we used std::unique_ptr, these lines would need to use get().
> 
> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:419
> > +        unum_close(numberFormat);
> 
> We would not need this if we used std::unique_ptr.
> 
> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:423
> > +    m_numberFormat = numberFormat;
> 
> This line would need to be:
> 
>     m_numberFormat = WTFMove(numberFormat);

Done. Thanks. I will update other Intl classes to std::unique_ptr in a separate patch.

> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:436
> > +    if (bitwise_cast<uint64_t>(number) == bitwise_cast<uint64_t>(-0.0))
> > +        number = 0.0;
> 
> How is the bitwise_cast code better than:
> 
>     // Map negative zero to positive zero.
>     if (!number)
>         number = 0;
> 
> I don’t think it is.

Fixed.

> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:447
> > +        return state.vm().throwException(&state, createError(&state, ASCIILiteral("Failed to format a number.")));
> 
> Where is the test coverage for this? When can this fail?

The ICU spec doesn't say what can cause unum_formatUFormattable to fail, and I can't find a case that makes it fail. I put the check in there just in case.

-- 
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/20160215/c363f8e3/attachment.html>