Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Editorial: Alphabetize extension keys. #433

Merged
merged 2 commits into from
May 18, 2020

Conversation

rkirsling
Copy link
Member

Resolves #427.

The first commit alphabetizes all [[RelevantExtensionKeys]] slots, which has no normative impact due to UTS 35 canonicalization. It does not change the order of internal slots (though I suppose it could) or resolved options (which would make this normative).

The second commit here alphabetizes non-normative mentions of extension keys throughout 402.

@sffc
Copy link
Contributor

sffc commented May 4, 2020

@jswalden @FrankYFTang

@FrankYFTang FrankYFTang self-requested a review May 5, 2020 00:23
Copy link
Contributor

@FrankYFTang FrankYFTang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

caiolima pushed a commit to caiolima/webkit that referenced this pull request May 6, 2020
https://bugs.webkit.org/show_bug.cgi?id=211359

Reviewed by Darin Adler.

JSTests:

* stress/intl-collator.js:
* stress/intl-datetimeformat.js:
Add tests.

Source/JavaScriptCore:

Two cleanup items for Intl classes:

1. Ensure `resolvedOptions().locale` returns relevant extension keys in alphabetical order.
   ICU does this for us via Intl.getCanonicalLocales / Intl.*.supportedLocalesOf but not via ResolveLocale.
   However, we don't need to do any sorting in ResolveLocale; we can just pre-alphabetize relevantExtensionKeys.
   (See also tc39/ecma402#433.)

2. Ensure Intl classes are marking const methods correctly.

* runtime/IntlCollator.cpp:
(JSC::IntlCollator::sortLocaleData):
(JSC::IntlCollator::searchLocaleData):
(JSC::IntlCollator::compareStrings const): Add const specifier.
(JSC::IntlCollator::resolvedOptions const): Add const specifier.
* runtime/IntlCollator.h:
* runtime/IntlDateTimeFormat.cpp:
(JSC::IntlDateTimeFormat::localeData):
(JSC::IntlDateTimeFormat::resolvedOptions const): Add const specifier.
(JSC::IntlDateTimeFormat::format const): Add const specifier.
(JSC::IntlDateTimeFormat::formatToParts const): Add const specifier.
* runtime/IntlDateTimeFormat.h:
* runtime/IntlNumberFormat.cpp:
(JSC::IntlNumberFormat::format const): Add const specifier.
(JSC::IntlNumberFormat::resolvedOptions const): Add const specifier.
(JSC::IntlNumberFormat::formatToParts const): Add const specifier.
* runtime/IntlNumberFormat.h:
* runtime/IntlPluralRules.cpp:
(JSC::IntlPluralRules::resolvedOptions const): Add const specifier.
(JSC::IntlPluralRules::select const): Add const specifier.
* runtime/IntlPluralRules.h:
* runtime/IntlRelativeTimeFormat.cpp:
(JSC::IntlRelativeTimeFormat::resolvedOptions const): Add const specifier.
(JSC::IntlRelativeTimeFormat::formatInternal const): Add const specifier.
(JSC::IntlRelativeTimeFormat::format const): Add const specifier.
(JSC::IntlRelativeTimeFormat::formatToParts const): Add const specifier.
* runtime/IntlRelativeTimeFormat.h:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@261182 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Copy link
Member

@leobalter leobalter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

</p>

<emu-note>
Unicode Technical Standard 35 describes three locale extension keys that are relevant to date and time formatting, *"ca"* for calendar, *"tz"* for time zone, *"hc"* for hour cycle, and implicitly *"nu"* for the numbering system of the number format used for numbers within the date format. DateTimeFormat, however, requires that the time zone is specified through the *"timeZone"* property in the options objects.
Unicode Technical Standard 35 describes four locale extension keys that are relevant to date and time formatting: *"ca"* for calendar, *"hc"* for hour cycle, *"nu"* for numbering system (of formatted numbers), and *"tz"* for time zone. DateTimeFormat, however, requires that the time zone is specified through the *"timeZone"* property in the options objects.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

off-by-one

@leobalter leobalter merged commit 97c16d6 into tc39:master May 18, 2020
@rkirsling rkirsling deleted the alphabetize-extension-keys branch May 18, 2020 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should [[RelevantExtensionKeys]] be pre-alphabetized?
5 participants