1736834 - Modernise intl::Locale

Status: RESOLVED FIXED

(Core :: Internationalization, task)

Description

[André Bargull [:anba]]

The initial port of mozilla::intl::Locale kept some SpiderMonkey specific designs, which aren't necessarily appropriate for general uses.
For example:

• Locale::unicodeExtension() returns const char*, where nullptr indicates an absent Unicode extension. A better design for general purpose Locale is probably returning Maybe<Span<const char>>.
• Locale::addLikelySubtags() returns bool where false indicates the failure case instead of using ICUResult.
• The original code (bug 1570370) was created before bug 1583953, so I couldn't use operator== and operator< from mozilla::Span and instead used std::char_traits extensively.

Comment 1

[André Bargull [:anba]]

Attachment: Bug 1736834 - Part 1: Replace manual string comparisons with Span comparisons. r=#platform-i18n-reviewers!

The original code was created before bug 1583953 and at that point of time, the
Span operators where still costly to call due to release-assertions overhead.

With the release assertions removed, the Span operators are now just as efficient
as manually calling std::char_traits. (Both will compile down to plain memcmp
calls.) But compared to using std::char_traits, the Span operators are more
ergonomic to use and result in less written code.

Depends on D129036

Comment 2

[André Bargull [:anba]]

Attachment: Bug 1736834 - Part 2: Resultify Locale::{add,remove}LikelySubtags(). r=#platform-i18n-reviewers!

Return ICUResult from Locale::{add,remove}LikelySubtags() instead of using bool.

Depends on D129037

Comment 3

[André Bargull [:anba]]

Attachment: Bug 1736834 - Part 3: Resultify Locale::setUnicodeExtension(). r=#platform-i18n-reviewers!

Return ICUResult from Locale::setUnicodeExtension() instead of using bool.

Depends on D129038

Comment 4

[André Bargull [:anba]]

Attachment: Bug 1736834 - Part 4: Change Locale::setUnicodeExtension() to accept a Span. r=#platform-i18n-reviewers!

This avoids unnecessary null-termination for the callers.

Depends on D129039

Comment 5

[André Bargull [:anba]]

Attachment: Bug 1736834 - Part 5: Use the existing ICUResult typedef for Locale::toString(). r=#platform-i18n-reviewers!

Depends on D129040

Comment 6

[André Bargull [:anba]]

Attachment: Bug 1736834 - Part 6: Use Maybe instead of nullptr to represent absent Unicode extensions. r=#platform-i18n-reviewers!

Use Maybe and return Span instead of raw const char*.

Depends on D129041

Comment 7

[André Bargull [:anba]]

Attachment: Bug 1736834 - Part 7: Use Maybe instead of nullptr to represent absent private-use subtags. r=#platform-i18n-reviewers!

Use Maybe and return Span instead of raw const char*.

Depends on D129042

Comment 8

[André Bargull [:anba]]

Attachment: Bug 1736834 - Part 8: Use direct member access for Locale::{variants_, extensions_}. r=#platform-i18n-reviewers!

Use direct member access in preparation for the next part.

Depends on D129043

Comment 9

[André Bargull [:anba]]

Attachment: Bug 1736834 - Part 9: Expose Locale::{variants_,extension_} as an iterable structure. r=#platform-i18n-reviewers!

Expose both members only as an iterable structure to hide implementation details
like the UniqueChars storage.

Depends on D129044

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c6d8f836668e
Part 1: Replace manual string comparisons with Span comparisons. r=platform-i18n-reviewers,dminor
https://hg.mozilla.org/integration/autoland/rev/198e41713023
Part 2: Resultify Locale::{add,remove}LikelySubtags(). r=platform-i18n-reviewers,dminor
https://hg.mozilla.org/integration/autoland/rev/34e727062e6c
Part 3: Resultify Locale::setUnicodeExtension(). r=platform-i18n-reviewers,dminor
https://hg.mozilla.org/integration/autoland/rev/aa05e4759185
Part 4: Change Locale::setUnicodeExtension() to accept a Span. r=platform-i18n-reviewers,dminor
https://hg.mozilla.org/integration/autoland/rev/1fea7abd8773
Part 5: Use the existing ICUResult typedef for Locale::toString(). r=platform-i18n-reviewers,dminor
https://hg.mozilla.org/integration/autoland/rev/29911ad4824e
Part 6: Use Maybe instead of nullptr to represent absent Unicode extensions. r=platform-i18n-reviewers,dminor
https://hg.mozilla.org/integration/autoland/rev/043a907d0d3f
Part 7: Use Maybe instead of nullptr to represent absent private-use subtags. r=platform-i18n-reviewers,dminor
https://hg.mozilla.org/integration/autoland/rev/6dd568c25660
Part 8: Use direct member access for Locale::{variants_, extensions_}. r=platform-i18n-reviewers,dminor
https://hg.mozilla.org/integration/autoland/rev/e64bbe494da4
Part 9: Expose Locale::{variants_,extension_} as an iterable structure. r=platform-i18n-reviewers,dminor

Comment 11

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/c6d8f836668e
https://hg.mozilla.org/mozilla-central/rev/198e41713023
https://hg.mozilla.org/mozilla-central/rev/34e727062e6c
https://hg.mozilla.org/mozilla-central/rev/aa05e4759185
https://hg.mozilla.org/mozilla-central/rev/1fea7abd8773
https://hg.mozilla.org/mozilla-central/rev/29911ad4824e
https://hg.mozilla.org/mozilla-central/rev/043a907d0d3f
https://hg.mozilla.org/mozilla-central/rev/6dd568c25660
https://hg.mozilla.org/mozilla-central/rev/e64bbe494da4