Bug 147605

Created attachment 270536 [details] WIP

Created attachment 271257 [details] Patch

Comment on attachment 271257 [details] Patch Attachment 271257 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/822892 New failing tests: js/intl-numberformat.html

Created attachment 271260 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 271257 [details] Patch Attachment 271257 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/822904 New failing tests: js/intl-numberformat.html

Created attachment 271261 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Comment on attachment 271257 [details] Patch Attachment 271257 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/822888 New failing tests: js/intl-numberformat.html

Created attachment 271262 [details] Archive of layout-test-results from ews113 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 271263 [details] Patch

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

Created attachment 271314 [details] Patch

Created attachment 271315 [details] Patch

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

Created attachment 271390 [details] Patch

Since Darin gave this r+, if you're just responding to his comments it's OK to add his name to the Reviewed by line and commit, unless you're looking for more feedback. If you're not a committer, you could reupload the patch and request only cq? Or: you could wait for Darin to see this comment and click the button again. :)

Created attachment 271828 [details] Patch for landing

Comment on attachment 271828 [details] Patch for landing Rejecting attachment 271828 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 271828, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in LayoutTests/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/856604

Created attachment 271830 [details] Patch for landing

Comment on attachment 271830 [details] Patch for landing Clearing flags on attachment: 271830 Committed r196850: <http://trac.webkit.org/changeset/196850>

All reviewed patches have been landed. Closing bug.