[webkit-reviews] review granted: [Bug 171709] [INTL] Use struct instead of HashMap for resolveLocale operation : [Attachment 311404] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Andy VanWagoner
<thetalecrafter at gmail.com>'s request for review:
Bug 171709: [INTL] Use struct instead of HashMap for resolveLocale operation
https://bugs.webkit.org/show_bug.cgi?id=171709

Attachment 311404: Patch

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




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 311404
  --> https://bugs.webkit.org/attachment.cgi?id=311404
Patch

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

> Source/JavaScriptCore/runtime/IntlObject.h:72
> +    const String& getExtension(Extension);

WebKit coding style prohibits the use of the word "get" in a function name like
this one. We would name it just "extension".

> Source/JavaScriptCore/runtime/IntlObject.h:73
> +    void setExtension(Extension, String);

Should take "const String&" instead of String to avoid reference count churn.

> Source/JavaScriptCore/runtime/IntlObject.h:83
> +void resolveLocale(ExecState&, const HashSet<String>& availableLocales,
const Vector<String>& requestedLocales, IntlResolvedLocale& options, const
IntlResolvedLocale::Extension relevantExtensionKeys[], size_t
relevantExtensionKeyCount, Vector<String> (*localeData)(const String&, const
IntlResolvedLocale::Extension));

The const here in const IntlResolvedLocale::Extension is not helpful. The use
of size_t for relevantExtensionKeyCount seems like overkill. We normally use
unsigned for such things in WebKit.

The use of in/out for options here, where options is the fourth of seven
arguments, makes this pretty hard to read. I wonder if there's a way to make
this more straightforward.