[webkit-reviews] review granted: [Bug 229892] [JSC] Implement Temporal.PlainTime : [Attachment 437524] 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 229892: [JSC] Implement Temporal.PlainTime
https://bugs.webkit.org/show_bug.cgi?id=229892

Attachment 437524: Patch

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




--- Comment #14 from Darin Adler <darin at apple.com> ---
Comment on attachment 437524
  --> https://bugs.webkit.org/attachment.cgi?id=437524
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?