-
Notifications
You must be signed in to change notification settings - Fork 106
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ryzokuken
approved these changes
May 3, 2020
FrankYFTang
approved these changes
May 5, 2020
There was a problem hiding this 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
leobalter
approved these changes
May 18, 2020
There was a problem hiding this 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
off-by-one
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.