- Use std::unique_ptr to store ICU objects (See Comment 10 in https://bugs.webkit.org/show_bug.cgi?id=147603#c10) - Pass Vector::size() to ICU functions that take a buffer size instead of Vector::capacity() - If U_SUCCESS(status) is true, it means there is no error, but there could be warnings. ICU functions ignore warnings. So, there is no need to reset status to U_ZERO_ERROR. - Remove the initialization of the String instance variables of IntlDateTimeFormat. These values are never read and cause unnecessary memory allocation. - Fix coding style
Created attachment 271835 [details] Patch
Oops, wrong link. For the comment about std::unique_ptr, see Comment 10 in https://bugs.webkit.org/show_bug.cgi?id=147605#c10
Comment on attachment 271835 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271835&action=review > Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:674 > + m_dateFormat = std::unique_ptr<UDateFormat, UDateFormatDeleter>(udat_open(UDAT_IGNORE, UDAT_IGNORE, m_locale.utf8().data(), timeZoneView.upconvertedCharacters(), timeZoneView.length(), pattern.upconvertedCharacters(), pattern.length(), &status)); Since ICU headers were updated to v52, UDAT_PATTERN is preferred over UDAT_IGNORE. Mind changing that while you are in here?
Created attachment 271839 [details] Patch
Created attachment 271840 [details] Patch
(In reply to comment #3) > Comment on attachment 271835 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=271835&action=review > > > Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:674 > > + m_dateFormat = std::unique_ptr<UDateFormat, UDateFormatDeleter>(udat_open(UDAT_IGNORE, UDAT_IGNORE, m_locale.utf8().data(), timeZoneView.upconvertedCharacters(), timeZoneView.length(), pattern.upconvertedCharacters(), pattern.length(), &status)); > > Since ICU headers were updated to v52, UDAT_PATTERN is preferred over > UDAT_IGNORE. Mind changing that while you are in here? Done. Thanks!
Comment on attachment 271840 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271840&action=review > Source/JavaScriptCore/runtime/IntlCollator.cpp:329 > + auto collator = std::unique_ptr<UCollator, UCollatorDeleter>(ucol_open(m_locale.utf8().data(), &status)); Use std::make_unique from StdLibExtras.h
(In reply to comment #7) > Comment on attachment 271840 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=271840&action=review > > > Source/JavaScriptCore/runtime/IntlCollator.cpp:329 > > + auto collator = std::unique_ptr<UCollator, UCollatorDeleter>(ucol_open(m_locale.utf8().data(), &status)); > > Use std::make_unique from StdLibExtras.h I don't think that works, because ucol_open is not a C++ constructor.
Comment on attachment 271840 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271840&action=review >>> Source/JavaScriptCore/runtime/IntlCollator.cpp:329 >>> + auto collator = std::unique_ptr<UCollator, UCollatorDeleter>(ucol_open(m_locale.utf8().data(), &status)); >> >> Use std::make_unique from StdLibExtras.h > > I don't think that works, because ucol_open is not a C++ constructor. That’s right. Can’t use make_unique here.
Comment on attachment 271840 [details] Patch Clearing flags on attachment: 271840 Committed r196887: <http://trac.webkit.org/changeset/196887>
All reviewed patches have been landed. Closing bug.