Bug 154486 – Improvements to Intl code

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Sukolsak Sakshuwong]

- 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

[Sukolsak Sakshuwong]

Created attachment 271835 [details]
Patch

[Sukolsak Sakshuwong]

Oops, wrong link. For the comment about std::unique_ptr, see Comment 10 in https://bugs.webkit.org/show_bug.cgi?id=147605#c10

[Andy VanWagoner]

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?

[Sukolsak Sakshuwong]

Created attachment 271839 [details]
Patch

[Sukolsak Sakshuwong]

Created attachment 271840 [details]
Patch

[Sukolsak Sakshuwong]

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

[Michael Catanzaro]

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

[Sukolsak Sakshuwong]

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

[Darin Adler]

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.

[WebKit Commit Bot]

Comment on attachment 271840 [details]
Patch

Clearing flags on attachment: 271840

Committed r196887: <http://trac.webkit.org/changeset/196887>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.