Summary: | [INTL] Implement Number Format Functions | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andy VanWagoner <andy> | ||||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | benjamin, buildbot, commit-queue, darin, ggaren, keith_miller, mark.lam, mcatanzaro, msaboff, rniwa, saam, sukolsak | ||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=246646 | ||||||||||||||||||||||||||
Bug Depends on: | 154160, 154530, 154533 | ||||||||||||||||||||||||||
Bug Blocks: | 90906, 152033 | ||||||||||||||||||||||||||
Attachments: |
|
Description
Andy VanWagoner
2015-08-03 17:21:58 PDT
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. |