Bug 214446 - [JSC] Clean up resolveLocale
Summary: [JSC] Clean up resolveLocale
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-07-17 00:08 PDT by Yusuke Suzuki
Modified: 2020-07-21 10:18 PDT (History)
9 users (show)

See Also:


Attachments
Patch (36.47 KB, patch)
2020-07-17 00:09 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (36.83 KB, patch)
2020-07-17 00:19 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (37.16 KB, patch)
2020-07-17 02:18 PDT, Yusuke Suzuki
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2020-07-17 00:08:38 PDT
[JSC] Clean up resolveLocale
Comment 1 Yusuke Suzuki 2020-07-17 00:09:35 PDT
Created attachment 404541 [details]
Patch
Comment 2 Yusuke Suzuki 2020-07-17 00:19:12 PDT
Created attachment 404544 [details]
Patch
Comment 3 Yusuke Suzuki 2020-07-17 02:18:02 PDT
Created attachment 404552 [details]
Patch
Comment 4 Darin Adler 2020-07-17 08:03:26 PDT
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.
Comment 5 Yusuke Suzuki 2020-07-17 12:08:10 PDT
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.
Comment 6 Yusuke Suzuki 2020-07-17 15:05:41 PDT
Committed r264537: <https://trac.webkit.org/changeset/264537>
Comment 7 Radar WebKit Bug Importer 2020-07-17 15:06:17 PDT
<rdar://problem/65746090>
Comment 8 Caio Lima 2020-07-21 09:08:01 PDT
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.
Comment 9 Darin Adler 2020-07-21 09:09:02 PDT
So it sounds like ASSERT_UNDER_CONSTEXPR_CONTEXT does not yet work on these compilers. Or is this somehow specific to this file?
Comment 10 Yusuke Suzuki 2020-07-21 09:30:35 PDT
(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.
Comment 11 Yusuke Suzuki 2020-07-21 10:18:54 PDT
(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).