[webkit-reviews] review granted: [Bug 214446] [JSC] Clean up resolveLocale : [Attachment 404552] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 214446: [JSC] Clean up resolveLocale
https://bugs.webkit.org/show_bug.cgi?id=214446

Attachment 404552: Patch

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




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

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

> Source/JavaScriptCore/runtime/IntlCollator.cpp:84
> +Vector<String> IntlCollator::sortLocaleData(const String& locale,
RelevantExtensionKey relevantExtensionKey)

I suggest considering a shorter argument name for this argument in this
context, where things aren’t ambiguous. Maybe "key" or "extension" or
"extensionKey".

> Source/JavaScriptCore/runtime/IntlCollator.cpp:190
> +	   localeOptions[static_cast<unsigned>(RelevantExtensionKey::Kn)] =
String(numeric == TriState::True ? "true"_s : "false"_s);

I’d like us to make a constexpr function in the header to convert to an integer
type so that the understanding that it is safe to cast is in exactly one place,
rather than having to think about that question in each place where we do a
static_cast.

Why is the explicit conversion to String needed?

> Source/JavaScriptCore/runtime/IntlCollator.h:28
> +#include "IntlObject.h"

Can we use a forward declaration of RelevantExtensionKey instead of adding an
include? I think those work for enum class.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.h:28
> +#include "IntlObject.h"

Can we use a forward declaration of RelevantExtensionKey instead of adding an
include? I think those work for enum class.

> Source/JavaScriptCore/runtime/IntlNumberFormat.h:29
> +#include "IntlObject.h"

Can we use a forward declaration of RelevantExtensionKey instead of adding an
include? I think those work for enum class.

> Source/JavaScriptCore/runtime/IntlObject.cpp:632
> +inline ASCIILiteral relevantExtensionKeyString(RelevantExtensionKey key)

Seems like this could be constexpr, not just inline.

> Source/JavaScriptCore/runtime/IntlObject.cpp:691
> +	       if ((optionsValue->isNull() ||
keyLocaleData.contains(optionsValue.value())) && optionsValue.value() != value)
{

I often prefer the * syntax to the value() syntax. Is that an option here?

> Source/JavaScriptCore/runtime/IntlObject.h:56
> +static constexpr unsigned numberOfRelevantExtensionKeys = 0
JSC_INTL_RELEVANT_EXTENSION_KEYS(JSC_COUNT_INTL_RELEVANT_EXTENSION_KEYS);

Seems like this should be a uint8_t to be consistent with the type used for the
enumeration itself. Since this is going to be roughly the same size.

> Source/JavaScriptCore/runtime/IntlObject.h:101
> +    String locale;
> +    String dataLocale;

Can either of these be ASCIILiteral instead of String?

> Source/JavaScriptCore/runtime/IntlObject.h:105
> +ResolvedLocale resolveLocale(JSGlobalObject*, const HashSet<String>&
availableLocales, const Vector<String>& requestedLocales, LocaleMatcher, const
ResolveLocaleOptions&, std::initializer_list<RelevantExtensionKey>
relevantExtensionKeys, Vector<String> (*localeData)(const String&,
RelevantExtensionKey));

Can any of this be ASCIILiteral instead of String?

> Source/JavaScriptCore/runtime/IntlPluralRules.h:28
> +#include "IntlObject.h"

Can we use a forward declaration of RelevantExtensionKey instead of adding an
include? I think those work for enum class.