Bug 210457 – [ECMA-402] Extension values should default to true, canonicalize without "-true"

• Component: New Bugs
• Status: RESOLVED FIXED

[Ross Kirsling]

[ECMA-402] Extension values should default to true, canonicalize without "-true"

[Ross Kirsling]

Created attachment 396338 [details]
Patch

[Ross Kirsling]

Created attachment 396345 [details]
Patch

[Ross Kirsling]

Comment on attachment 396345 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396345&action=review

> JSTests/stress/intl-collator.js:85
> -shouldBe(testCollator(Intl.Collator('en-u-kn'), [{locale: 'en', numeric: true}]), true);
> -shouldBe(testCollator(Intl.Collator('en-u-kn-true'), [{locale: 'en-u-kn-true', numeric: true}]), true);
> +shouldBe(testCollator(Intl.Collator('en-u-kn'), [{locale: 'en-u-kn', numeric: true}]), true);
> +shouldBe(testCollator(Intl.Collator('en-u-kn-true'), [{locale: 'en-u-kn', numeric: true}]), true);

These two test cases are basically the change in a nutshell.

[Ross Kirsling]

Created attachment 396564 [details]
Patch

Make ChangeLog clearer, fix string literal

[Yusuke Suzuki]

Comment on attachment 396564 [details]
Patch

r=me

[EWS]

Committed r260151: <https://trac.webkit.org/changeset/260151>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396564 [details].

[Radar WebKit Bug Importer]

<rdar://problem/61847887>

[Darin Adler]

Comment on attachment 396564 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396564&action=review

> Source/JavaScriptCore/runtime/IntlObject.cpp:539
> +            if (lowercase != "true"_s) {

There is no benefit of using _s here, but it also does no harm. Generates exactly the same code, I believe. I suppose the rules for when ASCIILiteral is helpful are too confusing.

> Source/JavaScriptCore/runtime/IntlObject.cpp:541
> +                extension.append('-');
> +                extension.append(lowercase);

This one-liner is more efficient than two append calls.

    extension = makeString(extension, '-', lowercase);

[Ross Kirsling]

(In reply to Darin Adler from comment #8)
> Comment on attachment 396564 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396564&action=review
> 
> > Source/JavaScriptCore/runtime/IntlObject.cpp:539
> > +            if (lowercase != "true"_s) {
> 
> There is no benefit of using _s here, but it also does no harm. Generates
> exactly the same code, I believe. I suppose the rules for when ASCIILiteral
> is helpful are too confusing.

This one was Yusuke's suggestion. :)

> > Source/JavaScriptCore/runtime/IntlObject.cpp:541
> > +                extension.append('-');
> > +                extension.append(lowercase);
> 
> This one-liner is more efficient than two append calls.
> 
>     extension = makeString(extension, '-', lowercase);

Oh, that's good to know. I wasn't sure whether `foo = makeString(foo, ...)` was a good pattern or not. Will update.

[Ross Kirsling]

(In reply to Ross Kirsling from comment #9)
> Oh, that's good to know. I wasn't sure whether `foo = makeString(foo, ...)`
> was a good pattern or not. Will update.

Committed r260161: <https://trac.webkit.org/changeset/260161>