Bug 214446 – [JSC] Clean up resolveLocale

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Clean up resolveLocale

[Yusuke Suzuki]

Created attachment 404541 [details]
Patch

[Yusuke Suzuki]

Created attachment 404544 [details]
Patch

[Yusuke Suzuki]

Created attachment 404552 [details]
Patch

[Darin Adler]

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

[Yusuke Suzuki]

Comment on attachment 404552 [details]
Patch

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

Thanks!

>> 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".

Changed!

>> 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?

Because this is `Optional<String>`, not `String`, so implicit conversion cannot happen. And the reason why Optional<String> is required is described in ChangeLog.

>> 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.

Changed.

>> 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.

Changed.

>> 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.

Changed.

>> Source/JavaScriptCore/runtime/IntlObject.cpp:632
>> +inline ASCIILiteral relevantExtensionKeyString(RelevantExtensionKey key)
> 
> Seems like this could be constexpr, not just inline.

Right. Changed.

>> 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?

It is Optional<>. I personally like `value()` since it is easy to grep, but yeah, but not so strong preference. Changed.

>> 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.

Changed.

>> Source/JavaScriptCore/runtime/IntlObject.h:101
>> +    String dataLocale;
> 
> Can either of these be ASCIILiteral instead of String?

They are modified and dynamically allocated Strings so we cannot use ASCIILiteral here.

>> 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?

Strings here is modified and dynamically allocated Strings, so we cannot use ASCIILiteral.

>> 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.

Changed.

[Yusuke Suzuki]

Committed r264537: <https://trac.webkit.org/changeset/264537>

[Radar WebKit Bug Importer]

<rdar://problem/65746090>

[Caio Lima]

Comment on attachment 404552 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/IntlObject.cpp:632
>>> +inline ASCIILiteral relevantExtensionKeyString(RelevantExtensionKey key)
>> 
>> Seems like this could be constexpr, not just inline.
> 
> Right. Changed.

The usage of constexpr here is breaking debug builds on linux using `gcc (Debian 8.3.0-6) 8.3.0`. I'm also getting similar error trying to build 32-bits ARMv7 and MIPS:

```
FAILED: Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/__/__/DerivedSources/JavaScriptCore/unified-sources/UnifiedSource-f2e18ffc-15.cpp.o 
/usr/bin/ccache /usr/bin/c++  -DBUILDING_JSCONLY__ -DBUILDING_JavaScriptCore -DBUILDING_WITH_CMAKE=1 -DHAVE_CONFIG_H=1 -DJavaScriptCore_EXPORTS -DSTATICALLY_LINKED_WITH_WTF -IDerivedSources/ForwardingHeaders -I. -I../../Source/JavaScriptCore -I../../Source/JavaScriptCore/API -I../../Source/JavaScriptCore/assembler -I../../Source/JavaScriptCore/b3 -I../../Source/JavaScriptCore/b3/air -I../../Source/JavaScriptCore/bindings -I../../Source/JavaScriptCore/builtins -I../../Source/JavaScriptCore/bytecode -I../../Source/JavaScriptCore/bytecompiler -I../../Source/JavaScriptCore/dfg -I../../Source/JavaScriptCore/disassembler -I../../Source/JavaScriptCore/disassembler/ARM64 -I../../Source/JavaScriptCore/disassembler/udis86 -I../../Source/JavaScriptCore/domjit -I../../Source/JavaScriptCore/ftl -I../../Source/JavaScriptCore/heap -I../../Source/JavaScriptCore/debugger -I../../Source/JavaScriptCore/inspector -I../../Source/JavaScriptCore/inspector/agents -I../../Source/JavaScriptCore/inspector/augmentable -I../../Source/JavaScriptCore/inspector/remote -I../../Source/JavaScriptCore/interpreter -I../../Source/JavaScriptCore/jit -I../../Source/JavaScriptCore/llint -I../../Source/JavaScriptCore/parser -I../../Source/JavaScriptCore/profiler -I../../Source/JavaScriptCore/runtime -I../../Source/JavaScriptCore/tools -I../../Source/JavaScriptCore/wasm -I../../Source/JavaScriptCore/wasm/js -I../../Source/JavaScriptCore/yarr -IDerivedSources/JavaScriptCore -IDerivedSources/JavaScriptCore/inspector -IDerivedSources/JavaScriptCore/runtime -IDerivedSources/JavaScriptCore/yarr -fdiagnostics-color=always -Wextra -Wall -Wno-expansion-to-defined -Wno-noexcept-type -Wno-psabi -Wno-maybe-uninitialized -Wwrite-strings -Wundef -Wpointer-arith -Wmissing-format-attribute -Wformat-security -Wcast-align  -fno-strict-aliasing -fno-exceptions -fno-rtti -gsplit-dwarf -g -fPIC   -ffp-contract=off -std=c++17 -MD -MT Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/__/__/DerivedSources/JavaScriptCore/unified-sources/UnifiedSource-f2e18ffc-15.cpp.o -MF Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/__/__/DerivedSources/JavaScriptCore/unified-sources/UnifiedSource-f2e18ffc-15.cpp.o.d -o Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/__/__/DerivedSources/JavaScriptCore/unified-sources/UnifiedSource-f2e18ffc-15.cpp.o -c DerivedSources/JavaScriptCore/unified-sources/UnifiedSource-f2e18ffc-15.cpp
In file included from DerivedSources/ForwardingHeaders/wtf/StdLibExtras.h:32,
                 from DerivedSources/ForwardingHeaders/wtf/FastMalloc.h:26,
                 from ../../Source/JavaScriptCore/config.h:38,
                 from ../../Source/JavaScriptCore/runtime/IntlLocaleConstructor.cpp:26,
                 from DerivedSources/JavaScriptCore/unified-sources/UnifiedSource-f2e18ffc-15.cpp:1:
../../Source/JavaScriptCore/runtime/IntlObject.cpp: In function ‘constexpr WTF::ASCIILiteral JSC::relevantExtensionKeyString(JSC::RelevantExtensionKey)’:
DerivedSources/ForwardingHeaders/wtf/Assertions.h:354:34: error: call to non-‘constexpr’ function ‘void WTFReportAssertionFailure(const char*, int, const char*, const char*)’
         WTFReportAssertionFailure(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, #assertion); \
         ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../Source/JavaScriptCore/runtime/IntlObject.cpp:657:5: note: in expansion of macro ‘ASSERT_UNDER_CONSTEXPR_CONTEXT’
     ASSERT_UNDER_CONSTEXPR_CONTEXT(0);
```

I couldn't find the reason why it is not possible to use `ASSERT_UNDER_CONSTEXPR_CONTEXT(0);` there, but I would guess that might be some divergence in `WTFReportAssertionFailure` for linux that is causing this issue.

[Darin Adler]

So it sounds like ASSERT_UNDER_CONSTEXPR_CONTEXT does not yet work on these compilers. Or is this somehow specific to this file?

[Yusuke Suzuki]

(In reply to Darin Adler from comment #9)
> So it sounds like ASSERT_UNDER_CONSTEXPR_CONTEXT does not yet work on these
> compilers. Or is this somehow specific to this file?

My guess is that these compilers are not handling switch statement's reachability correctly, and failed to analyze that this is unreachable.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #10)
> (In reply to Darin Adler from comment #9)
> > So it sounds like ASSERT_UNDER_CONSTEXPR_CONTEXT does not yet work on these
> > compilers. Or is this somehow specific to this file?
> 
> My guess is that these compilers are not handling switch statement's
> reachability correctly, and failed to analyze that this is unreachable.

For now, I'll just remove ASSERT_UNDER_CONSTEXPR_CONTEXT(0).