Bug 229892 – [JSC] Implement Temporal.PlainTime

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

...

[Yusuke Suzuki]

Created attachment 437349 [details]
Patch

[Yusuke Suzuki]

Created attachment 437350 [details]
Patch

[Yusuke Suzuki]

Created attachment 437351 [details]
Patch

[Yusuke Suzuki]

Created attachment 437353 [details]
Patch

[Yusuke Suzuki]

Created attachment 437355 [details]
Patch

[Yusuke Suzuki]

Created attachment 437356 [details]
Patch

[Yusuke Suzuki]

Created attachment 437357 [details]
Patch

[Yusuke Suzuki]

Created attachment 437404 [details]
Patch

[Yusuke Suzuki]

Created attachment 437446 [details]
Patch

[Yusuke Suzuki]

Created attachment 437448 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 437448 [details]
Patch

Looks like the spec's definition of DifferenceTime, BalanceTime is not correct.

[Yusuke Suzuki]

Created attachment 437524 [details]
Patch

[Darin Adler]

Comment on attachment 437524 [details]
Patch

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

> Source/JavaScriptCore/runtime/ISO8601.cpp:52
> +std::optional<TimeZoneID> idForTimeZoneName(StringView string)
> +{
> +    const auto& timeZones = intlAvailableTimeZones();
> +    for (unsigned index = 0; index < timeZones.size(); ++index) {
> +        if (equalIgnoringASCIICase(timeZones[index], string))
> +            return index;
> +    }
> +    return std::nullopt;
> +}

I guess this is just moved code. But we would normally try to avoid linear search in a case like this. We could change intlAvailableTimeZones to be sorted so we can do a binary search. Or go even further in making an optimal algorithm if there are restrictions to time zone names so they’d fit, for example, in a 64-bit integer, ala the PackedASCIILowerCodes.

[Darin Adler]

Comment on attachment 437524 [details]
Patch

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

> Source/JavaScriptCore/runtime/ISO8601.cpp:359
> +    unsigned maxCount = std::min<unsigned>(buffer.lengthRemaining(), 9);

I would slightly prefer:

    unsigned maxCount = std::min(buffer.lengthRemaining(), 9u);

What do you think?

> Source/JavaScriptCore/runtime/ISO8601.cpp:401
> +    else if (*buffer == '-' || *buffer == 0x2212) {

Could use "minusSign" instead of 0x2212 throughout; it’s defined in CharacterNames.h. I think it would be slightly better.

> Source/JavaScriptCore/runtime/ISO8601.cpp:432
> +std::optional<Variant<Vector<LChar>, int64_t>> parseTimeZoneBracketedAnnotation(StringParsingBuffer<CharacterType>& buffer)

Why not marked static?

> Source/JavaScriptCore/runtime/ISO8601.cpp:584
> +bool canBeTimeZone(const StringParsingBuffer<CharacterType>& buffer, CharacterType character)

Why not marked static?

> Source/JavaScriptCore/runtime/ISO8601.cpp:613
> +std::optional<TimeZoneRecord> parseTimeZone(StringParsingBuffer<CharacterType>& buffer)

Why not marked static?

> Source/JavaScriptCore/runtime/ISO8601.cpp:661
> +std::optional<std::tuple<PlainTime, std::optional<TimeZoneRecord>>> parseTime(StringParsingBuffer<CharacterType>& buffer)

Why not marked static?

> Source/JavaScriptCore/runtime/ISO8601.cpp:681
> +inline unsigned daysInMonth(int32_t year, unsigned month)

In a .cpp file I would expect this to be marked static, and not marked inline. The compiler will inline it without an inline keyword, right?

Although I would not do inline, I might be tempted to mark it constexpr because it’s that kind of function; should do the same with isLeapYear. But still would want "static".

> Source/JavaScriptCore/runtime/ISO8601.cpp:706
> +std::optional<PlainDate> parseDate(StringParsingBuffer<CharacterType>& buffer)

Why not marked static?

> Source/JavaScriptCore/runtime/ISO8601.cpp:826
> +std::optional<std::tuple<PlainDate, std::optional<PlainTime>, std::optional<TimeZoneRecord>>> parseDateTime(StringParsingBuffer<CharacterType>& buffer)

Why not marked static?

> Source/JavaScriptCore/runtime/ISO8601.cpp:922
> +    int64_t fractionNanoseconds = static_cast<int64_t>(plainTime.millisecond()) * nsPerMillisecond + static_cast<int64_t>(plainTime.microsecond()) * nsPerMicrosecond + static_cast<int64_t>(plainTime.nanosecond());

Can we use local variables instead of static_cast to expand to int64_t without a typecast?

> Source/JavaScriptCore/runtime/ISO8601.cpp:926
> +        auto fraction = WTF::numberToStringUnsigned<Vector<LChar, 9>>(fractionNanoseconds);

The use of WTF:: here means we forgot a "using" that we should have included in the WTF header, I think?

> Source/JavaScriptCore/runtime/ISO8601.cpp:944
> +    auto fraction = WTF::numberToStringUnsigned<Vector<LChar, 9>>(fractionNanoseconds);

Ditto.

> Source/JavaScriptCore/runtime/ISO8601.h:86
> +        : m_hour(0)
> +        , m_minute(0)
> +        , m_second(0)

I don’t think we need these three.

> Source/JavaScriptCore/runtime/ISO8601.h:158
> +struct TimeZoneRecord {
> +    bool m_z { false };
> +    std::optional<int64_t> m_offset;
> +    Variant<Vector<LChar>, int64_t> m_nameOrOffset;
> +};

Since this is a struct, I suggest not using "m_" prefixes on the member names.

> Source/JavaScriptCore/runtime/ISO8601.h:161
> +std::optional<TimeZoneID> idForTimeZoneName(StringView);

Could call this parseTimeZoneName.

> Source/JavaScriptCore/runtime/ISO8601.h:164
> +enum class ValidateTimeZoneID { Yes, No };

I don’t see this used anywhere.

> Source/JavaScriptCore/runtime/IntlObject.cpp:1847
> +        const auto& timeZones = intlAvailableTimeZones();

I don’t think we need "const".

> Source/JavaScriptCore/runtime/IntlObject.cpp:1853
> +        for (unsigned index = 0; index < timeZones.size(); ++index) {
> +            if (timeZones[index] == "UTC"_s) {
> +                utcTimeZoneIDStorage = index;
> +                return;
> +            }
> +        }

Can we just use Vector::find? Do we really need to write out a loop?

> Source/JavaScriptCore/runtime/IntlObject.h:128
> +inline CalendarID utcTimeZoneID()
> +{
> +    unsigned value = utcTimeZoneIDStorage;
> +    if (value == std::numeric_limits<TimeZoneID>::max())
> +        return utcTimeZoneIDSlow();
> +    return value;
> +}

Generally I prefer to put function bodies after declarations in a separate section of the header. Could we do that? Generally we want to separate interface from implementation even if both are in the same header.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1239
> +                JSGlobalObject* globalObject = jsCast<JSGlobalObject*>(init.owner);

auto?

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1240
> +                TemporalPlainTimePrototype* plainTimePrototype = TemporalPlainTimePrototype::create(init.vm, globalObject, TemporalPlainTimePrototype::createStructure(init.vm, globalObject, globalObject->objectPrototype()));

auto?

> Source/JavaScriptCore/runtime/TemporalObject.cpp:69
> +    TemporalObject* temporalObject = jsCast<TemporalObject*>(object);
> +    JSGlobalObject* globalObject = temporalObject->globalObject(vm);

auto?

> Source/JavaScriptCore/runtime/TemporalObject.h:84
> +enum class Precision {

Consider : uint8_t?

> Source/JavaScriptCore/runtime/TemporalObject.h:105
> +enum class TemporalOverflow : uint8_t {

Consider bool instead of uint8_t?

> Source/JavaScriptCore/runtime/TemporalPlainTime.cpp:420
> +        JSString* calendarString = calendar->toString(globalObject);
> +        RETURN_IF_EXCEPTION(scope, { });
> +        String calendarWTFString = calendarString->value(globalObject);

Why not use toWTFString in cases like this?

[Yusuke Suzuki]

Comment on attachment 437524 [details]
Patch

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

Thanks

>> Source/JavaScriptCore/runtime/ISO8601.cpp:52
>> +}
> 
> I guess this is just moved code. But we would normally try to avoid linear search in a case like this. We could change intlAvailableTimeZones to be sorted so we can do a binary search. Or go even further in making an optimal algorithm if there are restrictions to time zone names so they’d fit, for example, in a 64-bit integer, ala the PackedASCIILowerCodes.

Unfortunately, there is no limitation of the length of the TimeZone string (e.g. ISO8601 TimeZone name part can accept very long string by using many slashes, and TimeZone data gets longer name recently for some regions), and the data comes from ICU, so it is hard to pre-define the max length in our side.
I would like to do refactoring separately since this TimeZone strings are sorted by case-sensitive ordering, on the other hand, here, we need to use case-insensitive comparison. So it is possible that this vector is not sorted as we thought.
And that vector was sorted because we are using this sorted vector for Intl.supportedValuesOf("timeZone"). So we cannot change the ordering without changing that function too, and I think this should be done in a separate patch from this Temporal.PlainTime patch.

>> Source/JavaScriptCore/runtime/ISO8601.cpp:359
>> +    unsigned maxCount = std::min<unsigned>(buffer.lengthRemaining(), 9);
> 
> I would slightly prefer:
> 
>     unsigned maxCount = std::min(buffer.lengthRemaining(), 9u);
> 
> What do you think?

Looks good!

>> Source/JavaScriptCore/runtime/ISO8601.cpp:401
>> +    else if (*buffer == '-' || *buffer == 0x2212) {
> 
> Could use "minusSign" instead of 0x2212 throughout; it’s defined in CharacterNames.h. I think it would be slightly better.

Sounds good. Changed.

>> Source/JavaScriptCore/runtime/ISO8601.cpp:432
>> +std::optional<Variant<Vector<LChar>, int64_t>> parseTimeZoneBracketedAnnotation(StringParsingBuffer<CharacterType>& buffer)
> 
> Why not marked static?

Fixed

>> Source/JavaScriptCore/runtime/ISO8601.cpp:584
>> +bool canBeTimeZone(const StringParsingBuffer<CharacterType>& buffer, CharacterType character)
> 
> Why not marked static?

Fixed

>> Source/JavaScriptCore/runtime/ISO8601.cpp:613
>> +std::optional<TimeZoneRecord> parseTimeZone(StringParsingBuffer<CharacterType>& buffer)
> 
> Why not marked static?

Fixed

>> Source/JavaScriptCore/runtime/ISO8601.cpp:661
>> +std::optional<std::tuple<PlainTime, std::optional<TimeZoneRecord>>> parseTime(StringParsingBuffer<CharacterType>& buffer)
> 
> Why not marked static?

Fixed

>> Source/JavaScriptCore/runtime/ISO8601.cpp:681
>> +inline unsigned daysInMonth(int32_t year, unsigned month)
> 
> In a .cpp file I would expect this to be marked static, and not marked inline. The compiler will inline it without an inline keyword, right?
> 
> Although I would not do inline, I might be tempted to mark it constexpr because it’s that kind of function; should do the same with isLeapYear. But still would want "static".

Yup, adding static.

>> Source/JavaScriptCore/runtime/ISO8601.cpp:706
>> +std::optional<PlainDate> parseDate(StringParsingBuffer<CharacterType>& buffer)
> 
> Why not marked static?

Fixed

>> Source/JavaScriptCore/runtime/ISO8601.cpp:826
>> +std::optional<std::tuple<PlainDate, std::optional<PlainTime>, std::optional<TimeZoneRecord>>> parseDateTime(StringParsingBuffer<CharacterType>& buffer)
> 
> Why not marked static?

Fixed

>> Source/JavaScriptCore/runtime/ISO8601.cpp:922
>> +    int64_t fractionNanoseconds = static_cast<int64_t>(plainTime.millisecond()) * nsPerMillisecond + static_cast<int64_t>(plainTime.microsecond()) * nsPerMicrosecond + static_cast<int64_t>(plainTime.nanosecond());
> 
> Can we use local variables instead of static_cast to expand to int64_t without a typecast?

Changed.

>> Source/JavaScriptCore/runtime/ISO8601.cpp:926
>> +        auto fraction = WTF::numberToStringUnsigned<Vector<LChar, 9>>(fractionNanoseconds);
> 
> The use of WTF:: here means we forgot a "using" that we should have included in the WTF header, I think?

Added using.

>> Source/JavaScriptCore/runtime/ISO8601.cpp:944
>> +    auto fraction = WTF::numberToStringUnsigned<Vector<LChar, 9>>(fractionNanoseconds);
> 
> Ditto.

Changed.

>> Source/JavaScriptCore/runtime/ISO8601.h:86
>> +        , m_second(0)
> 
> I don’t think we need these three.

Removed.

>> Source/JavaScriptCore/runtime/ISO8601.h:158
>> +};
> 
> Since this is a struct, I suggest not using "m_" prefixes on the member names.

In JSC, we would like to add m_ prefix to struct fields too in recent JSC development largely because of improved grep-ability.
This thing improved our BytecodeStruct related operations' readability. (https://trac.webkit.org/changeset/240041/webkit)

>> Source/JavaScriptCore/runtime/ISO8601.h:161
>> +std::optional<TimeZoneID> idForTimeZoneName(StringView);
> 
> Could call this parseTimeZoneName.

Changed.

>> Source/JavaScriptCore/runtime/ISO8601.h:164
>> +enum class ValidateTimeZoneID { Yes, No };
> 
> I don’t see this used anywhere.

Removed.

>> Source/JavaScriptCore/runtime/IntlObject.cpp:1847
>> +        const auto& timeZones = intlAvailableTimeZones();
> 
> I don’t think we need "const".

Removed.

>> Source/JavaScriptCore/runtime/IntlObject.cpp:1853
>> +        }
> 
> Can we just use Vector::find? Do we really need to write out a loop?

We can use find. Changed.

>> Source/JavaScriptCore/runtime/IntlObject.h:128
>> +}
> 
> Generally I prefer to put function bodies after declarations in a separate section of the header. Could we do that? Generally we want to separate interface from implementation even if both are in the same header.

OK, changed.

>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1239
>> +                JSGlobalObject* globalObject = jsCast<JSGlobalObject*>(init.owner);
> 
> auto?

Changed.

>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1240
>> +                TemporalPlainTimePrototype* plainTimePrototype = TemporalPlainTimePrototype::create(init.vm, globalObject, TemporalPlainTimePrototype::createStructure(init.vm, globalObject, globalObject->objectPrototype()));
> 
> auto?

Changed.

>> Source/JavaScriptCore/runtime/TemporalObject.cpp:69
>> +    JSGlobalObject* globalObject = temporalObject->globalObject(vm);
> 
> auto?

Changed.

>> Source/JavaScriptCore/runtime/TemporalObject.h:84
>> +enum class Precision {
> 
> Consider : uint8_t?

Changed.

>> Source/JavaScriptCore/runtime/TemporalObject.h:105
>> +enum class TemporalOverflow : uint8_t {
> 
> Consider bool instead of uint8_t?

Changed.

>> Source/JavaScriptCore/runtime/TemporalPlainTime.cpp:420
>> +        String calendarWTFString = calendarString->value(globalObject);
> 
> Why not use toWTFString in cases like this?

Because toWTFString is only for JSValue :) (no definition for JSObject*).

[Yusuke Suzuki]

Committed r282125 (241422@main): <https://commits.webkit.org/241422@main>

[Radar WebKit Bug Importer]

<rdar://problem/82851167>

[Yusuke Suzuki]

Committed r282127 (241424@main): <https://commits.webkit.org/241424@main>

[Darin Adler]

Comment on attachment 437524 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/ISO8601.h:158
>>> +};
>> 
>> Since this is a struct, I suggest not using "m_" prefixes on the member names.
> 
> In JSC, we would like to add m_ prefix to struct fields too in recent JSC development largely because of improved grep-ability.
> This thing improved our BytecodeStruct related operations' readability. (https://trac.webkit.org/changeset/240041/webkit)

While I understand that you all have decided on this, I am not sure it makes sense to me. Why not put a prefix on function names too so for improved grep-ability?

[Yusuke Suzuki]

Comment on attachment 437524 [details]
Patch

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

>>>> Source/JavaScriptCore/runtime/ISO8601.h:158
>>>> +};
>>> 
>>> Since this is a struct, I suggest not using "m_" prefixes on the member names.
>> 
>> In JSC, we would like to add m_ prefix to struct fields too in recent JSC development largely because of improved grep-ability.
>> This thing improved our BytecodeStruct related operations' readability. (https://trac.webkit.org/changeset/240041/webkit)
> 
> While I understand that you all have decided on this, I am not sure it makes sense to me. Why not put a prefix on function names too so for improved grep-ability?

This is because `m_xxx` is always a member name while `xxx` can be function name, local variable name, global variable name, etc.
Grepping with `m_z` is much easier than grepping with `z` for example. (in this case :))

[Darin Adler]

Comment on attachment 437524 [details]
Patch

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

>>>>> Source/JavaScriptCore/runtime/ISO8601.h:158
>>>>> +};
>>>> 
>>>> Since this is a struct, I suggest not using "m_" prefixes on the member names.
>>> 
>>> In JSC, we would like to add m_ prefix to struct fields too in recent JSC development largely because of improved grep-ability.
>>> This thing improved our BytecodeStruct related operations' readability. (https://trac.webkit.org/changeset/240041/webkit)
>> 
>> While I understand that you all have decided on this, I am not sure it makes sense to me. Why not put a prefix on function names too so for improved grep-ability?
> 
> This is because `m_xxx` is always a member name while `xxx` can be function name, local variable name, global variable name, etc.
> Grepping with `m_z` is much easier than grepping with `z` for example. (in this case :))

Yes, but function names could all be "f_z" too.

[Yusuke Suzuki]

Comment on attachment 437524 [details]
Patch

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

>>>>>> Source/JavaScriptCore/runtime/ISO8601.h:158
>>>>>> +};
>>>>> 
>>>>> Since this is a struct, I suggest not using "m_" prefixes on the member names.
>>>> 
>>>> In JSC, we would like to add m_ prefix to struct fields too in recent JSC development largely because of improved grep-ability.
>>>> This thing improved our BytecodeStruct related operations' readability. (https://trac.webkit.org/changeset/240041/webkit)
>>> 
>>> While I understand that you all have decided on this, I am not sure it makes sense to me. Why not put a prefix on function names too so for improved grep-ability?
>> 
>> This is because `m_xxx` is always a member name while `xxx` can be function name, local variable name, global variable name, etc.
>> Grepping with `m_z` is much easier than grepping with `z` for example. (in this case :))
> 
> Yes, but function names could all be "f_z" too.

Yeah, but function can be (relatively) excluded with `f_z(` grep :)

[Michael Catanzaro]

So this commit introduced a few GCC build warnings. There are a couple harmless -Wredundant-move caused by "return WTFMove(result)" on line 579. That's trivial: we can just remove the WTFMove. But we also have three cases of -Wswitch-outside-range. They look like this:

In file included from JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-f2e18ffc-13.cpp:2:
../../Source/JavaScriptCore/runtime/ISO8601.cpp: In instantiation of ‘bool JSC::ISO8601::canBeTimeZone(const WTF::StringParsingBuffer<CharacterType>&, CharacterType) [with CharacterType = unsigned char]’:
../../Source/JavaScriptCore/runtime/ISO8601.cpp:672:22:   required from ‘std::optional<std::tuple<JSC::ISO8601::PlainTime, std::optional<JSC::ISO8601::TimeZoneRecord> > > JSC::ISO8601::parseTime(WTF::StringParsingBuffer<CharacterType>&) [with CharacterType = unsigned char]’
../../Source/JavaScriptCore/runtime/ISO8601.cpp:863:32:   required from ‘JSC::ISO8601::parseTime(WTF::StringView)::<lambda(auto:28)> [with auto:28 = WTF::StringParsingBuffer<unsigned char>]’
WTF/Headers/wtf/text/StringParsingBuffer.h:117:23:   required from ‘decltype(auto) WTF::readCharactersForParsing(StringType&&, Function&&) [with StringType = WTF::StringView&; Function = JSC::ISO8601::parseTime(WTF::StringView)::<lambda(auto:28)>]’
../../Source/JavaScriptCore/runtime/ISO8601.cpp:862:36:   required from here
../../Source/JavaScriptCore/runtime/ISO8601.cpp:596:5: warning: case label value exceeds maximum value for type [-Wswitch-outside-range]
  596 |     case minusSign:
      |     ^~~~

It's also triggered by line 636 and line 454. Problem is, minusSign is declared as 0x2212 (in CharacterNames.h), which is outside the range of CharacterType when CharacterType is unsigned char. Of course when CharacterType is UChar, then it's no longer going to be outside the range. My proposal here is to use IGNORE_GCC_WARNINGS_BEGIN("switch-outside-range") and move on. Yusuke, do you agree?

[Yusuke Suzuki]

(In reply to Michael Catanzaro from comment #23)
> So this commit introduced a few GCC build warnings. There are a couple
> harmless -Wredundant-move caused by "return WTFMove(result)" on line 579.
> That's trivial: we can just remove the WTFMove. But we also have three cases
> of -Wswitch-outside-range. They look like this:
> 
> In file included from
> JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-f2e18ffc-13.cpp:
> 2:
> ../../Source/JavaScriptCore/runtime/ISO8601.cpp: In instantiation of ‘bool
> JSC::ISO8601::canBeTimeZone(const WTF::StringParsingBuffer<CharacterType>&,
> CharacterType) [with CharacterType = unsigned char]’:
> ../../Source/JavaScriptCore/runtime/ISO8601.cpp:672:22:   required from
> ‘std::optional<std::tuple<JSC::ISO8601::PlainTime,
> std::optional<JSC::ISO8601::TimeZoneRecord> > >
> JSC::ISO8601::parseTime(WTF::StringParsingBuffer<CharacterType>&) [with
> CharacterType = unsigned char]’
> ../../Source/JavaScriptCore/runtime/ISO8601.cpp:863:32:   required from
> ‘JSC::ISO8601::parseTime(WTF::StringView)::<lambda(auto:28)> [with auto:28 =
> WTF::StringParsingBuffer<unsigned char>]’
> WTF/Headers/wtf/text/StringParsingBuffer.h:117:23:   required from
> ‘decltype(auto) WTF::readCharactersForParsing(StringType&&, Function&&)
> [with StringType = WTF::StringView&; Function =
> JSC::ISO8601::parseTime(WTF::StringView)::<lambda(auto:28)>]’
> ../../Source/JavaScriptCore/runtime/ISO8601.cpp:862:36:   required from here
> ../../Source/JavaScriptCore/runtime/ISO8601.cpp:596:5: warning: case label
> value exceeds maximum value for type [-Wswitch-outside-range]
>   596 |     case minusSign:
>       |     ^~~~
> 
> It's also triggered by line 636 and line 454. Problem is, minusSign is
> declared as 0x2212 (in CharacterNames.h), which is outside the range of
> CharacterType when CharacterType is unsigned char. Of course when
> CharacterType is UChar, then it's no longer going to be outside the range.
> My proposal here is to use IGNORE_GCC_WARNINGS_BEGIN("switch-outside-range")
> and move on. Yusuke, do you agree?

Ugh, that looks not clean. How about changing `switch (character)` to `switch (static_cast<UChar>(character))`?

[Michael Catanzaro]

(In reply to Yusuke Suzuki from comment #24)
> Ugh, that looks not clean. How about changing `switch (character)` to
> `switch (static_cast<UChar>(character))`?

Good idea!

[Michael Catanzaro]

Follow-up: bug #230154