[webkit-reviews] review denied: [Bug 229826] [JSC] Implement Temporal.Instant : [Attachment 439756] Patch for preliminary review

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Oct 3 00:18:09 PDT 2021


Yusuke Suzuki <ysuzuki at apple.com> has denied Philip Chimento
<philip.chimento at gmail.com>'s request for review:
Bug 229826: [JSC] Implement Temporal.Instant
https://bugs.webkit.org/show_bug.cgi?id=229826

Attachment 439756: Patch for preliminary review

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




--- Comment #14 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 439756
  --> https://bugs.webkit.org/attachment.cgi?id=439756
Patch for preliminary review

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

Commented.
Please update DerivedSources-input.xcfilelist and
DerivedSources-output.xcfilelist too.

> Source/JavaScriptCore/runtime/ISO8601.cpp:907
> +	   return {ExactTime::fromISOPartsAndOffset(date.year(), date.month(),
date.day(), time.hour(), time.minute(), time.second(), time.millisecond(),
time.microsecond(), time.nanosecond(), offset)};

Add space around {.

> Source/JavaScriptCore/runtime/ISO8601.cpp:1017
> +    double utcMs = (dateDays * msPerDay + timeMs);

() is not necessary.

> Source/JavaScriptCore/runtime/ISO8601.cpp:1018
> +    Int128 utcNs = Int128 {static_cast<int64_t>(utcMs)} * 1'000'000 + micros
* 1000 + ns;

Add space around {.

> Source/JavaScriptCore/runtime/ISO8601.cpp:1019
> +    return ExactTime {utcNs - offset};

Add space around {.

> Source/JavaScriptCore/runtime/ISO8601.cpp:1038
> +    resultNs += Int128 {static_cast<int64_t>(duration.hours())} *
ExactTime::nsPerHour;

Add space around {.

> Source/JavaScriptCore/runtime/ISO8601.cpp:1041
> +    resultNs += Int128 {static_cast<int64_t>(duration.minutes())} *
ExactTime::nsPerMinute;

Add space around {.

> Source/JavaScriptCore/runtime/ISO8601.cpp:1044
> +    resultNs += Int128 {static_cast<int64_t>(duration.seconds())} *
ExactTime::nsPerSecond;

Add space around {.

> Source/JavaScriptCore/runtime/ISO8601.cpp:1047
> +    resultNs += Int128 {static_cast<int64_t>(duration.milliseconds())} *
ExactTime::nsPerMillisecond;

Add space around {.

> Source/JavaScriptCore/runtime/ISO8601.cpp:1050
> +    resultNs += Int128 {static_cast<int64_t>(duration.microseconds())} *
ExactTime::nsPerMicrosecond;

Add space around {.

> Source/JavaScriptCore/runtime/ISO8601.cpp:1051
> +    resultNs += Int128 {static_cast<int64_t>(duration.nanoseconds())};

Add space around {.

> Source/JavaScriptCore/runtime/ISO8601.cpp:1053
> +    ExactTime result {resultNs};

Add space around {.

> Source/JavaScriptCore/runtime/ISO8601.cpp:1056
> +    return {result};

{} is not necessary.

> Source/JavaScriptCore/runtime/ISO8601.cpp:1061
> +    Int128 incrementNs {increment};

Add space around {.

> Source/JavaScriptCore/runtime/ISO8601.cpp:1085
> +    return ExactTime {round(m_epochNanoseconds, increment, unit,
roundingMode)};

Add space around {.

> Source/JavaScriptCore/runtime/ISO8601.h:88
> +    constexpr ExactTime(const ExactTime& other) :
m_epochNanoseconds(other.m_epochNanoseconds) { }

This is not necessary.

> Source/JavaScriptCore/runtime/ISO8601.h:92
> +	   return ExactTime(Int128 {epochSeconds} * ExactTime::nsPerSecond);

Add space around {.

> Source/JavaScriptCore/runtime/ISO8601.h:96
> +	   return ExactTime(Int128 {epochMilliseconds} *
ExactTime::nsPerMillisecond);

Add space around {.

> Source/JavaScriptCore/runtime/ISO8601.h:100
> +	   return ExactTime(Int128 {epochMicroseconds} *
ExactTime::nsPerMicrosecond);

Add space around {.

> Source/JavaScriptCore/runtime/ISO8601.h:139
> +    String print() const
> +    {
> +	   StringBuilder builder;
> +	   if (m_epochNanoseconds < 0) {
> +	       builder.append('-');
> +	       printImpl(builder, -m_epochNanoseconds);
> +	   } else
> +	       printImpl(builder, m_epochNanoseconds);
> +	   return builder.toString();
> +    }

"print" function should be used for printing string to dataLog. Please rename
it.

> Source/JavaScriptCore/runtime/ISO8601.h:181
> +    static constexpr Int128 dayRangeSeconds {86400'00000000}; // 1e8 days
> +    static constexpr Int128 nsPerMicrosecond {1000};
> +    static constexpr Int128 nsPerMillisecond {1'000'000};
> +    static constexpr Int128 nsPerSecond {1'000'000'000};

Add space around { and }.

> Source/JavaScriptCore/runtime/IntlDateTimeFormatPrototype.cpp:99
> +	   return dateNowImpl().toNumber(globalObject);

Use RELEASE_AND_RETURN(scope, dateNowImpl().toNumber(globalObject));

> Source/JavaScriptCore/runtime/IntlDateTimeFormatPrototype.cpp:111
> +    auto scope = DECLARE_THROW_SCOPE(vm);

VM& vm = globalObject->vm();
auto scope = DECLARE_THROW_SCOPE(vm);
should be defined in the prologue of the function.

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:58
> +void TemporalInstant::finishCreation(VM& vm)
> +{
> +    Base::finishCreation(vm);
> +    ASSERT(inherits(vm, info()));
> +}

We do not need to define it.

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:91
> +    JSBigInt* bigint = asHeapBigInt(epochNanoseconds);

Add BIGINT32 case.

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:94
> +    ISO8601::ExactTime exactTime {(high << 64 | low) * (bigint->sign() ? -1
: 1)};

Add space around {.

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:97
> +	   String argAsString {bigint->toString(globalObject, 10)};

In this case, use argsAsString = bigint->toString(globalObject, 10)

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:120
> +    // TODO: when Temporal.ZonedDateTime lands

TODO is not used in WebKit. Use FIXME

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:200
> +    JSBigInt* bigint = asHeapBigInt(epochMicroseconds);

Add BIGINT32 case.

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:222
> +    VM& vm = globalObject->vm();
> +    auto scope = DECLARE_THROW_SCOPE(vm);
> +

This is not necessary.

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:281
> +    double seconds {static_cast<double>(static_cast<int64_t>(roundedDiff /
1'000'000'000))};

Add space around {.

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:282
> +    double nanosecondsRemainder
{static_cast<double>(static_cast<int64_t>(roundedDiff % 1'000'000'000))};

Add space around {.

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:283
> +    ISO8601::Duration result {0, 0, 0, 0, 0, 0, seconds, 0, 0,
nanosecondsRemainder};

Add space around {.

> Source/JavaScriptCore/runtime/TemporalInstant.cpp:406
> +    char buffer[20];
> +    // If the year is outside the bounds of 0 and 9999 inclusive we want to
use
> +    // the extended year format (PadISOYear).
> +    int charactersWritten;
> +    if (gregorianDateTime.year() > 9999 || gregorianDateTime.year() < 1000)
> +	   charactersWritten = snprintf(buffer, sizeof(buffer),
"%+07d-%02d-%02dT%02d:%02d", gregorianDateTime.year(),
gregorianDateTime.month() + 1, gregorianDateTime.monthDay(),
gregorianDateTime.hour(), gregorianDateTime.minute());
> +    else
> +	   charactersWritten = snprintf(buffer, sizeof(buffer),
"%04d-%02d-%02dT%02d:%02d", gregorianDateTime.year(), gregorianDateTime.month()
+ 1, gregorianDateTime.monthDay(), gregorianDateTime.hour(),
gregorianDateTime.minute());
> +    ASSERT(charactersWritten > 0 && static_cast<unsigned>(charactersWritten)
< sizeof(buffer));
> +    if (static_cast<unsigned>(charactersWritten) >= sizeof(buffer))
> +	   return ""_s;
> +
> +    StringBuilder builder;
> +    builder.append(buffer);

Let's not use snprintf. It is very slow compared to the other way. You can use
makeString, pad() etc. instead.

> Source/JavaScriptCore/runtime/TemporalInstantPrototype.cpp:121
> +    const std::optional<ISO8601::ExactTime> newExactTime =
instant->exactTime().add(duration);

We don't use const in this case.

> Source/JavaScriptCore/runtime/TemporalInstantPrototype.cpp:142
> +    const std::optional<ISO8601::ExactTime> newExactTime =
instant->exactTime().add(-duration);

We don't use const in this case.


More information about the webkit-reviews mailing list