Normative: Perform a single "dateAdd" lookup for MoveRelativeDate #2267
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #2267 +/- ##
==========================================
- Coverage 95.01% 91.06% -3.96%
==========================================
Files 20 19 -1
Lines 10833 10528 -305
Branches 1937 1699 -238
==========================================
- Hits 10293 9587 -706
- Misses 504 929 +425
+ Partials 36 12 -24
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Ms2ger
changed the title
ptomato
approved these changes
Jun 30, 2022
ptomato
added
spec-text
gibson042
approved these changes
Jul 6, 2022
`CalendarDateAdd` was changed to avoid repeated lookups of `"dateAdd"`, but we still repeatedly lookup `"dateAdd"` when `CalendarDateAdd` is called from `MoveRelativeDate`.
…lativeDate calls Add `dateAdd` to all remaining `MoveRelativeDate` calls.
All callers are now passing `dateAdd`, so the parameter no longer needs to be optional.
…dDateTime The reference code was out of line with the spec text here, which says that ToTemporalTimeZone must be called before InterpretTemporalDateTimeFields. Both calls are observable. See also tc39#2252
….p.total The reference code was out of line with the spec text here, which says that ToRelativeTemporalObject must be called before GetTemporalUnit. Both calls are observable. See also tc39#2252
…TemporalPlainYearMonth The reference code was out of line with the spec text here, which says that CalendarDateFromFields must be called the first time before the second call to DifferenceTemporalPlainYearMonth. These are observable. See also tc39#2252
This implements the normative change from "Normative: Add optional `dateAdd` parameter to MoveRelativeDate" in the reference code. It eliminates observable Gets of the calendar's dateAdd property each time MoveRelativeDate is called.
ptomato
added a commit
to ptomato/test262
that referenced
this pull request
Sep 26, 2022
See tc39/proposal-temporal#2267 which eliminated some unnecessary lookups of the calendar's dateAdd method from the MoveRelativeDate AO, which is called in the calendar types' since() and until() methods, Duration.p.round(), and Duration.p.total(). This adds tests for the order of all observable operations for these 10 methods, not just the lookups of dateAdd on the calendar. (These methods needed to have their order of observable operations tested anyway.) They heavily use the TemporalHelpers.calendarObserver and TemporalHelpers.timeZoneObserver added in the previous commits. tc39/proposal-temporal#2267 included changes to several code paths, not all of which are reachable through every method tested here; only Duration.p.round() can trigger the full set. tc39/proposal-temporal#2267 reached consensus at the July 2022 TC39 plenary meeting.
ptomato
added a commit
to ptomato/test262
that referenced
this pull request
Sep 26, 2022
See tc39/proposal-temporal#2267 which eliminated some unnecessary lookups of the calendar's dateAdd method from the MoveRelativeDate AO, which is called in the calendar types' since() and until() methods, Duration.p.round(), and Duration.p.total(). This adds tests for the order of all observable operations for these 10 methods, not just the lookups of dateAdd on the calendar. (These methods needed to have their order of observable operations tested anyway.) They heavily use the TemporalHelpers.calendarObserver and TemporalHelpers.timeZoneObserver added in the previous commits. tc39/proposal-temporal#2267 included changes to several code paths, not all of which are reachable through every method tested here; only Duration.p.round() can trigger the full set. tc39/proposal-temporal#2267 reached consensus at the July 2022 TC39 plenary meeting.
ptomato
force-pushed
the
move-relative-date-dateadd
branch
from
48b51d3 to
4e7e352
Compare
This implements the normative change from "Normative: Perform a single "dateAdd" lookup for the remaining MoveRelativeDate calls" in the reference code. It eliminates any remaining places where there were subsequent dateAdd lookups in the same abstract operation.
This implements the editorial change from "Editorial: Change `dateAdd` to a required parameter of MoveRelativeDate" in the reference code.
ptomato
force-pushed
the
move-relative-date-dateadd
branch
from
4e7e352 to
4ad6505
Compare
|
This reached consensus at the July 2022 TC39 meeting. Tests are in tc39/test262#3674. I've added an implementation of Anba's normative change to the reference code, as well as fixed a couple of observable-order bugs in the reference code that I ran into while writing the tests. |
ptomato
added a commit
to ptomato/test262
that referenced
this pull request
Oct 4, 2022
See tc39/proposal-temporal#2267 which eliminated some unnecessary lookups of the calendar's dateAdd method from the MoveRelativeDate AO, which is called in the calendar types' since() and until() methods, Duration.p.round(), and Duration.p.total(). This adds tests for the order of all observable operations for these 10 methods, not just the lookups of dateAdd on the calendar. (These methods needed to have their order of observable operations tested anyway.) They heavily use the TemporalHelpers.calendarObserver and TemporalHelpers.timeZoneObserver added in the previous commits. tc39/proposal-temporal#2267 included changes to several code paths, not all of which are reachable through every method tested here; only Duration.p.round() can trigger the full set. tc39/proposal-temporal#2267 reached consensus at the July 2022 TC39 plenary meeting.
Ms2ger
pushed a commit
to ptomato/test262
that referenced
this pull request
Oct 5, 2022
See tc39/proposal-temporal#2267 which eliminated some unnecessary lookups of the calendar's dateAdd method from the MoveRelativeDate AO, which is called in the calendar types' since() and until() methods, Duration.p.round(), and Duration.p.total(). This adds tests for the order of all observable operations for these 10 methods, not just the lookups of dateAdd on the calendar. (These methods needed to have their order of observable operations tested anyway.) They heavily use the TemporalHelpers.calendarObserver and TemporalHelpers.timeZoneObserver added in the previous commits. tc39/proposal-temporal#2267 included changes to several code paths, not all of which are reachable through every method tested here; only Duration.p.round() can trigger the full set. tc39/proposal-temporal#2267 reached consensus at the July 2022 TC39 plenary meeting.
Ms2ger
pushed a commit
to tc39/test262
that referenced
this pull request
Oct 5, 2022
See tc39/proposal-temporal#2267 which eliminated some unnecessary lookups of the calendar's dateAdd method from the MoveRelativeDate AO, which is called in the calendar types' since() and until() methods, Duration.p.round(), and Duration.p.total(). This adds tests for the order of all observable operations for these 10 methods, not just the lookups of dateAdd on the calendar. (These methods needed to have their order of observable operations tested anyway.) They heavily use the TemporalHelpers.calendarObserver and TemporalHelpers.timeZoneObserver added in the previous commits. tc39/proposal-temporal#2267 included changes to several code paths, not all of which are reachable through every method tested here; only Duration.p.round() can trigger the full set. tc39/proposal-temporal#2267 reached consensus at the July 2022 TC39 plenary meeting.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CalendarDateAddwas changed to avoid repeated lookups of"dateAdd", but we still repeatedly lookup"dateAdd"whenCalendarDateAddis called fromMoveRelativeDate.