Project C++ enums to Intl.js, implement Intl.Collator's "usage" option #5651

jackhorton · 2018-08-28T23:12:48Z

Also, some assorted fixes and cleanup:

We now correctly set the initSlotCapacity of the Intl native interface object
Moved around some of the asserts in GetLocaleData to be more defensive. None hit, just noticed they had gaps.
Added caching to Number.prototype.toLocaleString, which is now ~O(1) when calling with default arguments (locally on a debug build, time spent calling toLocaleString 100 times went from 140ms to 20ms)

Fixes #5648
Fixes #4007

sethbrenith

jefgen

LGTM, thanks Jack.
cc: @Microsoft/target-dev as FYI.

jefgen · 2018-08-30T22:40:32Z

lib/Runtime/Library/IntlEngineInterfaceExtensionObject.cpp

+            int collationsCount = uenum_count(collations, &status);
+
+            // we expect collationsCount to have at least "standard" and "search" in it
+            ICU_ASSERT(status, collationsCount > 2);


I don't if it is guaranteed that all locales will have 2 (unique to the locale) collation types. I think that in some cases the "search" might be aliased to the "default/standard" collation type.
However, that said, theres fallback to the root data well, so they might always have at least two, but based on falling back to root.

While debugging, I printed out all of the collation values for a few different locales and they all had at least two; if they don't, then our logic below could try to create a JavascriptArray of -1 elements (only if it has 0 collation values, which maybe is fine?), which would just failfast elsewhere (I hope...)

The Intl spec says that every locale has SearchLocaleData and SortLocaleData and usage must accept both search and sort for all locales, so I think the API was designed with the assumption that everyone would have both. It seems like a rare enough case, at least, that I would be interested in being alerted via a defensive failfast if at some point it doesn't hold.

Thanks Jack. To clarify I mean that not every individual locale (in and of itself) would necessarily have multiple collation types. (Like for example "en" might not anything specific to that locale at all).
Since there is fallback to the root data, the net effect is that yes I think everybody would have multiple.

jefgen · 2018-08-30T22:41:49Z

lib/Runtime/Library/IntlEngineInterfaceExtensionObject.cpp

+            const char16 searchString[] = _u("search");
+            if (usage->BufferEquals(searchString, _countof(searchString) - 1)) // minus the null terminator
+            {
+                uloc_setKeywordValue("collation", "search", localeID, _countof(localeID), &status);


Hmm, yeah, it is probably better to only set the "search" type, that way the defaults for the locale would be used for the "standard" type.

Yeah, the intl spec isn't super clear about whether usage=search or -u-co takes precendence since its working at a different abstraction level with the SortLocaleData and SearchLocaleData; if nothing else, SpiderMonkey refuses to accept -u-co when usage == search

Yeah, it does seem a bit odd.. I was wondering about the following scenario, though perhaps it is an edge case.
For example, what if (somehow) the default user locale was set to something other than the standard type. Then if the usage is "standard", would it make sense to change it to force the standard type? If yes, then how would you get the "default" type? Seems a bit odd.

Though I guess you have to include the locale in the parameters first, before you can specify the options -- so maybe my scenario doesn't make sense, unless you can somehow pass "und" as the locale, and it picks up the host OS default locale.

Also good point about what takes precedence as well.
Ex:
Intl.Collator('de-DE-u-co-phonebk', { usage: 'search' })

Will this give "search" type collation, or "phonebook" type collation?

Sorry for any confusion -- it was more of a question to @jackhorton about how the code/change currently works in the pull-request at the moment.

Right now the code only looks to see if the caller of the Intl API user passed in the "usage: search" in the options bag. If the caller passes in "usage: sort" then it doesn't explicitly set anything, and will use the locale/language tag "as-is".

My question was perhaps it should also explicitly set the collation keyword in the "usage: sort" case as well?

My understanding was that to ICU, en@collation=standard was ~equivalent to en, so we don't need to do anything special in that case (since standard === sort === the default for both Intl and ICU)

Right, what about something like this:
new Intl.Collator('es-u-co-trad', { usage: 'sort' })

If "sort === standard", and the options bag takes precedence, it sounds like in the above case you should be effectively getting 'es-u-co-standard' instead of 'traditional' -- right?

Oh... everyone (chakra/v8/sm) currently agrees that results in locale === es-u-co-trad and usage === sort. But I see your point that if usage is specified it should override -u-co, even if the specified value is sort... I can bring it up on the original tc39 issue that prompted me to implement usage, or bring it up at the meeting.

I can bring it up on the original tc39 issue that prompted me to implement usage, or bring it up at the meeting.

Yeah, would be good to discuss it. I guess the issue is that we don't want to alter unicode extensions you passed as we give them back in resolvedOptions. But maybe we should remove them? (those are the locales we matched, and those are the options we matched)?

dilijev

LGTM

…them in sync

… setting for Intl.Collator

…l.Collator's "usage" option Merge pull request #5651 from jackhorton:intl/projected-enums-and-usage Also, some assorted fixes and cleanup: - We now correctly set the initSlotCapacity of the Intl native interface object - Moved around some of the asserts in GetLocaleData to be more defensive. None hit, just noticed they had gaps. - Added caching to Number.prototype.toLocaleString, which is now ~O(1) when calling with default arguments (locally on a debug build, time spent calling toLocaleString 100 times went from 140ms to 20ms) Fixes #5648 Fixes #4007

jackhorton added Intl-ICU Bytecode-Update This PR updates bytecode and will cause merge conflicts with other PRs with this label labels Aug 28, 2018

jackhorton requested review from dilijev, MSLaguana and jefgen August 28, 2018 23:12

jackhorton force-pushed the intl/projected-enums-and-usage branch from f590d1b to ee1b45a Compare August 28, 2018 23:19

jackhorton mentioned this pull request Aug 28, 2018

What should happen with the usage: "sort" vs "search" choice in implementations? tc39/ecma402#256

Closed

MSLaguana approved these changes Aug 29, 2018

View reviewed changes

sethbrenith approved these changes Aug 30, 2018

View reviewed changes

jefgen approved these changes Aug 30, 2018

View reviewed changes

rhuanjl mentioned this pull request Sep 1, 2018

Implement Object.fromEntries #5622

Merged

dilijev approved these changes Sep 1, 2018

View reviewed changes

jackhorton added 4 commits September 6, 2018 08:33

Project C++ enums to Intl.js rather than hardcoding them and keeping …

b987205

…them in sync

Project BuiltInFunctionIDs to Intl.js and correctly respect the usage…

5cc1df9

… setting for Intl.Collator

Update bytecode

a7a7644

Update bytecode version

962624d

jackhorton force-pushed the intl/projected-enums-and-usage branch from ee1b45a to 962624d Compare September 6, 2018 17:15

chakrabot merged commit 962624d into chakra-core:master Sep 6, 2018

jackhorton deleted the intl/projected-enums-and-usage branch September 6, 2018 20:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Project C++ enums to Intl.js, implement Intl.Collator's "usage" option #5651

Project C++ enums to Intl.js, implement Intl.Collator's "usage" option #5651

jackhorton commented Aug 28, 2018 •

edited

sethbrenith left a comment

jefgen left a comment

jefgen Aug 30, 2018 •

edited

jackhorton Aug 30, 2018

jefgen Aug 30, 2018

jefgen Aug 30, 2018

jackhorton Aug 30, 2018

jefgen Aug 30, 2018 •

edited

jefgen Aug 30, 2018

jefgen Aug 30, 2018

jefgen Aug 31, 2018

jackhorton Aug 31, 2018

jefgen Aug 31, 2018

jackhorton Aug 31, 2018

zbraniecki Aug 31, 2018

dilijev left a comment

Project C++ enums to Intl.js, implement Intl.Collator's "usage" option #5651

Project C++ enums to Intl.js, implement Intl.Collator's "usage" option #5651

Conversation

jackhorton commented Aug 28, 2018 • edited

sethbrenith left a comment

Choose a reason for hiding this comment

jefgen left a comment

Choose a reason for hiding this comment

jefgen Aug 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jefgen Aug 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dilijev left a comment

Choose a reason for hiding this comment

jackhorton commented Aug 28, 2018 •

edited

jefgen Aug 30, 2018 •

edited

jefgen Aug 30, 2018 •

edited