[Webkit-unassigned] [Bug 229826] [JSC] Implement Temporal.Instant
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun Oct 3 00:18:09 PDT 2021
https://bugs.webkit.org/show_bug.cgi?id=229826
Yusuke Suzuki <ysuzuki at apple.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #439756|review? |review-
Flags| |
--- 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.
--
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20211003/9825789f/attachment-0001.htm>
More information about the webkit-unassigned
mailing list