[webkit-reviews] review granted: [Bug 227830] [JSC] Intl Locale Info : [Attachment 435948] Patch

[bugzilla-daemon at webkit.org]

Ross Kirsling <ross.kirsling at sony.com> has granted Yusuke Suzuki
<ysuzuki at apple.com>'s request for review:
Bug 227830: [JSC] Intl Locale Info
https://bugs.webkit.org/show_bug.cgi?id=227830

Attachment 435948: Patch

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




--- Comment #7 from Ross Kirsling <ross.kirsling at sony.com> ---
Comment on attachment 435948
  --> https://bugs.webkit.org/attachment.cgi?id=435948
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?