REGRESSION(r260697): [Intl] ""missing script" locales like zh-TW are no longer mapped
Created attachment 401510 [details] Patch
Comment on attachment 401510 [details] Patch Not thrilled that the test coverage exactly matches our hard-coded exception list. Is there a way to figure out what is expected and desired and make a broader test, even if some of the less important cases fail at this time? Generally not thrilled with tests that only test the positive cases.
Is there a reason this isn’t covered in Web Platform Tests?
(In reply to Darin Adler from comment #2) > Comment on attachment 401510 [details] > Patch > > Not thrilled that the test coverage exactly matches our hard-coded exception > list. Is there a way to figure out what is expected and desired and make a > broader test, even if some of the less important cases fail at this time? > > Generally not thrilled with tests that only test the positive cases. Like the ChangeLog indicates, there's an ongoing discussion about what the right set of locales here should be (since SpiderMonkey has the same hardcoded list): https://github.com/tc39/ecma402/issues/159 I got worried about sitting on this regression for too long (because not a few users will be upset if a mainline release goes out this way) so this is just meant as a partial revert, but I also figured that not adding a test would be a bad look. That said, I think my comment that "we can't just add xx-ZZ for every available xx-Yyyy-ZZ locale" may be wrong. Just because ICU4C doesn't have an explicit xx-ZZ available doesn't mean that it won't fall back properly. I should try this general approach after all. (In reply to Darin Adler from comment #3) > Is there a reason this isn’t covered in Web Platform Tests? It should be covered in test262 pending the resolution of the above discussion. (There are some tests indirectly assuming that zh-TW is a Hant locale, but...)
Created attachment 401605 [details] Patch
Uploaded a new version which keeps the regression test but doesn't use a hardcoded locale list. I think this is better but I hope I'm doing the createStaticStringImpl part correctly; we're still filling a NeverDestroyed<HashSet<String>> but now some of these are of our own creation (and some duplicate adds will occur).
Comment on attachment 401605 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401605&action=review > Source/JavaScriptCore/runtime/IntlObject.cpp:197 > + Vector<StringView> subtags; Make this Vector<StringView, 3> to avoid one memory allocation. > Source/JavaScriptCore/runtime/IntlObject.cpp:198 > + for (auto subtag : locale.splitAllowingEmptyEntries('-')) Do you really want the "allowing empty entries" version? Seems wrong to me. > Source/JavaScriptCore/runtime/IntlObject.cpp:199 > + subtags.append(subtag); To cut down memory allocation a bit more, write: if (subtags.size() >= 3) return; Before the call to append. > Source/JavaScriptCore/runtime/IntlObject.cpp:211 > + Vector<char, 12> buffer; > + ASSERT(subtags[0].is8Bit() && subtags[0].isAllASCII()); > + buffer.append(reinterpret_cast<const char*>(subtags[0].characters8()), subtags[0].length()); > + buffer.append('-'); > + ASSERT(subtags[2].is8Bit() && subtags[2].isAllASCII()); > + buffer.append(reinterpret_cast<const char*>(subtags[2].characters8()), subtags[2].length()); > + > + availableLocales.add(StringImpl::createStaticStringImpl(buffer.data(), buffer.size())); If it wasn’t for this StaticString thing, we could get rid of all those asserts and just write: availableLocales.add(makeString(subtags[0], '-', subtags[2]); But, as last time, I don’t really understand why StaticString is important here.
Created attachment 401609 [details] Patch
Committed r262890: <https://trac.webkit.org/changeset/262890> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401609 [details].
<rdar://problem/64235612>
Comment on attachment 401609 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401609&action=review > Source/JavaScriptCore/runtime/IntlObject.cpp:204 > + if (subtags.size() < 3 || subtags[1].length() != 4 || subtags[2].length() > 3) Realized that this should be "!= 3" rather than "< 3" for clarity.
(In reply to Darin Adler from comment #11) > Comment on attachment 401609 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=401609&action=review > > > Source/JavaScriptCore/runtime/IntlObject.cpp:204 > > + if (subtags.size() < 3 || subtags[1].length() != 4 || subtags[2].length() > 3) > > Realized that this should be "!= 3" rather than "< 3" for clarity. Hmm, I changed this from `!=` to `<` when I added the loop-internal early out you suggested, aiming to underscore the fact that we'd be gone already if we tried to grow beyond 3. I can switch it if it's inadvertently confusing though?
I think the narrower condition is simply easier to understand and more logical. From a "policy" point of view, we only want to run the code below if the size is exactly 3, and thus that’s what the code should say.
Works for me. Committed r262974: <https://trac.webkit.org/changeset/262974>