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

Editorial: Convert Date-related equations into proper AOs #3092

Merged
merged 1 commit into from
Jul 20, 2023

Conversation

jmdyck
Copy link
Collaborator

@jmdyck jmdyck commented Jun 5, 2023

Currently, 15 Date-related operations are defined via <emu-eqn> elements:

  • Day
  • TimeWithinDay
  • DaysInYear
  • DayFromYear
  • TimeFromYear
  • YearFromTime
  • InLeapYear
  • MonthFromTime
  • DayWithinYear
  • DateFromTime
  • WeekDay
  • HourFromTime
  • MinFromTime
  • SecFromTime
  • msFromTime

This PR converts each into a 'proper' abstract operation (with a structured header and an <emu-alg>).

For the one-line equations, conversion to an <emu-alg> was straightforward: basically just insert a Return. But for the multi-line definitions (DaysInYear, InLeapYear, MonthFromTime, DateFromTime), the changes were more involved, so should probably get more review attention.

I reordered the definitions a little: they have a fairly strong define-before-use vibe, so I made that stronger. Specifically, I reversed the following use-before-definitions:

  • msPerDay used in Day
  • DayWithinYear used in MonthFromTime
  • {Hour,Min,Sec,ms}FromTime used in UTC

I also dissolved the existing sections (promoting the new AO sections to 21.4.1.*) because they didn't serve much purpose after creating the AO sections. This makes the diff fairly 'solid' even if you ignore whitespace.

I've made multiple commits in case that helps with review or with changing things, but I imagine
I'll squash it down to one before merging.


Things I noticed but didn't do:

  • msFromTime could be renamed to MillisecFromTime so that it gets an initial capital, per the naming convention for AOs.
  • DaysInYear could be inlined into InLeapYear (with simplification) because that's its only use.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Jun 5, 2023

@bakkot: ecmarkup is complaining that _y_ is not defined in

1. Return the largest integral Number _y_ (closest to +∞)
   such that TimeFromYear(_y_) ≤ _t_.

I'm guessing that the parenthetical is throwing it off, because it seems okay with other forms of
the _x_ such that {condition using _x_}.

So: Is that phrasing something that ecmarkup should be able to handle, or should I add [declared="y"]?

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated
Comment on lines 32239 to 32240
1. If _month_ is *11*<sub>𝔽</sub>, return _dayWithinYear_ - *333*<sub>𝔽</sub> - _inLeapYear_.
1. Assert: Control never reaches this step.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. If _month_ is *11*<sub>𝔽</sub>, return _dayWithinYear_ - *333*<sub>𝔽</sub> - _inLeapYear_.
1. Assert: Control never reaches this step.
1. Assert: _month_ is *11*<sub>𝔽</sub>.
1. Return _dayWithinYear_ - *333*<sub>𝔽</sub> - _inLeapYear_.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. It looks like there's a lot of opportunity here for doing arithmetic on the reals instead of Numbers. That would be a nice follow-up PR.

Copy link
Contributor

@syg syg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think making these things real AOs is a strict improvement, so lgtm based on that.

That said, and this is an existing issue, I find the 𝔽 and ℝ conversions in the arithmetic expressions not the easiest to read. Like, some of the constants are floats and some are reals, depending on the arithmetic expressions that were used. Can we make these consistent somehow in an editorial way? (Like, make all input into reals, then do all the arithmetic, then cast to float at the very end.)

@bakkot
Copy link
Contributor

bakkot commented Jul 19, 2023

So: Is that phrasing something that ecmarkup should be able to handle, or should I add [declared="y"]?

I think that's too specialized for ecmarkup, so you should use the declared escape hatch.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Jul 19, 2023

So: Is that phrasing something that ecmarkup should be able to handle, or should I add [declared="y"]?

I think that's too specialized for ecmarkup, so you should use the declared escape hatch.

Done!

<dd>It returns the year in which _t_ falls.</dd>
</dl>
<emu-alg>
1. [declared="y"] Return the largest integral Number _y_ (closest to +∞) such that TimeFromYear(_y_) ≤ _t_.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. [declared="y"] Return the largest integral Number _y_ (closest to +∞) such that TimeFromYear(_y_) ≤ _t_.
1. [declared="y"] Return the largest integral Number _y_ (closest to *+∞*<sub>𝔽</sub>) such that TimeFromYear(_y_) ≤ _t_.

As mentioned in editor call today. We can hold it for a separate PR too, if you want.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think it'll fit better in a follow-up. (Or might not be necessary, if things shift to ℝ-space.)

@michaelficarra michaelficarra added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Jul 20, 2023
@jmdyck
Copy link
Collaborator Author

jmdyck commented Jul 20, 2023

(force-pushed to squash it down to 1 commit)

original steps:
- Make "Time-related Constants" section
- Move the "Hours, Minutes, Second, and Milliseconds" section
- Move "DayWithinYear" to a different spot
- Convert all the single-line <emu-eqn>s into AO sections
- Convert multi-line <emu-eqn>s to AO sections
- Refactor multi-step algorithms
- Move new AOs 'up' one level
@ljharb ljharb merged commit 7178fa8 into tc39:main Jul 20, 2023
6 checks passed
@jmdyck jmdyck deleted the Date_eqns branch July 22, 2023 13:04
@justingrant
Copy link
Contributor

  • msFromTime could be renamed to MillisecFromTime so that it gets an initial capital, per the naming convention for AOs.
  • DaysInYear could be inlined into InLeapYear (with simplification) because that's its only use.

I assume there may be quite a bit of refactoring to these operations when Temporal is merged, for example to DRY some of the duplication between these AOs and https://tc39.es/proposal-temporal/#sec-date-equations. Making changes like the ones above is probably easy to do as part of that Temporal-related refactoring.

So I'd recommend waiting until then to make more changes to these AOs, both because making changes will be easier as part of the Temporal merge, and because making more changes to Temporal-related parts of 262 will add work to the merge. Thanks!

@ptomato
Copy link
Contributor

ptomato commented Oct 2, 2023

LGTM. It looks like there's a lot of opportunity here for doing arithmetic on the reals instead of Numbers. That would be a nice follow-up PR.

Further reading on this topic: #1087, #1564, tc39/proposal-temporal#2315, tc39/proposal-temporal#2518.

tl;dr: This follow-up would be appreciated, even before merging in Temporal, but probably not straightforward.

zhangenming pushed a commit to zhangenming/ecma262 that referenced this pull request Dec 22, 2023
original steps:
- Make "Time-related Constants" section
- Move the "Hours, Minutes, Second, and Milliseconds" section
- Move "DayWithinYear" to a different spot
- Convert all the single-line <emu-eqn>s into AO sections
- Convert multi-line <emu-eqn>s to AO sections
- Refactor multi-step algorithms
- Move new AOs 'up' one level
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial change ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants