Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Investigate using EpochNanoseconds and implement wherever possible #119

Open
nekevss opened this issue Dec 5, 2024 · 0 comments
Open

Investigate using EpochNanoseconds and implement wherever possible #119

nekevss opened this issue Dec 5, 2024 · 0 comments
Labels
C-internal Internal library improvements E-easy Easy issue to fix - Good for newcomers Good First Issue

Comments

@nekevss
Copy link
Member

nekevss commented Dec 5, 2024

In #116, the EpochNanoseconds new type was added. The introduction of EpochNanoseconds was fairly bare, as further mentioned in #115. We should look into using EpochNanoseconds more throughout the codebase where it makes sense according to the proposal.

@nekevss nekevss added E-easy Easy issue to fix - Good for newcomers C-internal Internal library improvements Good First Issue labels Dec 5, 2024
nekevss added a commit that referenced this issue Dec 8, 2024
…er (#128)

This fixes a bug that was introduced by #116.

General gist is that the in the proposal [`5.5.4 ISODateTimeWithinLimits
( isoDateTime
)`](https://tc39.es/proposal-temporal/#sec-temporal-isodatetimewithinlimits)
there is a call to `GetUTCEpochNanoseconds`. However, this value is not
checked to be valid, and instead of constraining to the
NS_MAX_INSTANT/NS_MIN_INSTANT +/- NS_PER_DAY, respectively, we were
constraining to NS_MAX_INSTANT/NS_MIN_INSTANT.

There was a limit test on datetime, but we were only asserting the
negative case, not the positive case, so the regression was not caught.

So adjusted to handle the potentially invalid value, adjusted the
preexisting datetime test, and added a date limit test based off the
test262 limit test.

EDIT: Potentially relevant context to #119
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-internal Internal library improvements E-easy Easy issue to fix - Good for newcomers Good First Issue
Projects
None yet
Development

No branches or pull requests

1 participant