[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