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

[bugzilla-daemon at webkit.org]

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

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #271263|review?                     |review+
              Flags|                            |

--- Comment #10 from Darin Adler <darin at apple.com> ---
Comment on attachment 271263
  --> https://bugs.webkit.org/attachment.cgi?id=271263
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);

> 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.

> 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?

-- 
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/20160214/b1e4813f/attachment.html>