Bug 171709 – [INTL] Use struct instead of HashMap for resolveLocale operation

• Component: JavaScriptCore
• Status: RESOLVED WONTFIX

[Andy VanWagoner]

[INTL] Use struct instead of HashMap for resolveLocale operation

[Andy VanWagoner]

Created attachment 309249 [details]
Patch

[JF Bastien]

Comment on attachment 309249 [details]
Patch

Sorry for being slow, I've been away for a while. Could you rebase? Some of the file diffs won't expand.

[Andy VanWagoner]

Created attachment 311404 [details]
Patch

[Darin Adler]

Comment on attachment 311404 [details]
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.

[Andy VanWagoner]

Sorry for the long silence. I'll take another pass at making the resolveLocale function easier to understand.

[Andy VanWagoner]

This is old and not necessary. If there's a good reason to refactor it later, we can create a new issue.