Bug 227830 – [JSC] Intl Locale Info

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

...

[Yusuke Suzuki]

Created attachment 433259 [details]
Patch

[Yusuke Suzuki]

https://github.com/tc39/proposal-intl-locale-info

[Yusuke Suzuki]

Created attachment 433274 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/80674634>

[Yusuke Suzuki]

Created attachment 435117 [details]
Patch

[Yusuke Suzuki]

Created attachment 435948 [details]
Patch

[Ross Kirsling]

Comment on attachment 435948 [details]
Patch

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

r=me with comments

> Source/JavaScriptCore/runtime/IntlLocale.cpp:540
> +static inline JSArray* createArrayFromStringVector(JSGlobalObject* globalObject, Vector<String, 1>&& elements)

Heh, I'm slightly surprised we don't already have something like this.

> Source/JavaScriptCore/runtime/IntlLocale.cpp:581
> +        // Ensure aliases used in language tag are allowed.

Maybe put in a shared location so that IntlDateTimeFormat doesn't have to store this knowledge separately?

> Source/JavaScriptCore/runtime/IntlLocale.cpp:627
> +        // Map keyword values to BCP 47 equivalents.

Maybe put in a shared location so that IntlCollator doesn't have to store this knowledge separately?

> Source/JavaScriptCore/runtime/IntlLocale.cpp:679
> +        unsigned count = 1;

Unused variable? (Seems like it's incremented but never checked.)

> Source/JavaScriptCore/runtime/IntlLocale.cpp:826
> +        case UCAL_WEEKEND:
> +            return UCAL_WEEKEND;
> +        default:
> +            return UCAL_WEEKEND;
> +        }
> +        return UCAL_WEEKEND;

Why not just a single `return UCAL_WEEKEND;` after the `default:`, instead of three?

[Yusuke Suzuki]

Comment on attachment 435948 [details]
Patch

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

>> Source/JavaScriptCore/runtime/IntlLocale.cpp:826
>> +        return UCAL_WEEKEND;
> 
> Why not just a single `return UCAL_WEEKEND;` after the `default:`, instead of three?

Removed the last one. But I think having

        case UCAL_WEEKEND_CEASE:
        case UCAL_WEEKEND:

is good for readability rather than using `default`, in particular when a new UCalendarWeekdayType is added to ICU.

[Yusuke Suzuki]

Comment on attachment 435948 [details]
Patch

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

>> Source/JavaScriptCore/runtime/IntlLocale.cpp:581
>> +        // Ensure aliases used in language tag are allowed.
> 
> Maybe put in a shared location so that IntlDateTimeFormat doesn't have to store this knowledge separately?

Done.

>> Source/JavaScriptCore/runtime/IntlLocale.cpp:627
>> +        // Map keyword values to BCP 47 equivalents.
> 
> Maybe put in a shared location so that IntlCollator doesn't have to store this knowledge separately?

Done

>> Source/JavaScriptCore/runtime/IntlLocale.cpp:679
>> +        unsigned count = 1;
> 
> Unused variable? (Seems like it's incremented but never checked.)

Removed.

[Yusuke Suzuki]

Committed r281374 (240789@main): <https://commits.webkit.org/240789@main>