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

LocalTimeZoneAdjustment/LocalTZA follow-ups necessary #1070

Open
anba opened this Issue Jan 17, 2018 · 11 comments

Comments

Projects
None yet
4 participants
@anba
Contributor

anba commented Jan 17, 2018

In addition to the fixes in #1053:

  • Some sentences should be simplified/split-up so they're easier to understand. For example instead of

    "LocalTZA( t, isUTC ) is an implementation-defined algorithm that must return a number representing milliseconds suitable for adding to a Time Value."

    we could just as well just write:

    "LocalTZA is an implementation-defined algorithm that returns the local time zone offset in milliseconds."

  • <dfn> seems to be used incorrectly a number of times (i.e. it's not used to define a term, but merely to change the formatting).

  • LocalTZA's t parameter is described as a time value. This is incorrect, t can be a number value outside of the allowed range for time values.

  • The fourth paragraph seems to imply that a daylight saving time offset is always positive. This is no longer true starting with tzdata2018a.

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan
Member

littledan commented Jan 17, 2018

@apaprocki

This comment has been minimized.

Show comment
Hide comment
@apaprocki

apaprocki Mar 21, 2018

Contributor

@anba I'm curious about your point about LocalTZA accepting Number and not the more restrictive time value. Why do you feel it should accept values outside of the maximum time value range?

Contributor

apaprocki commented Mar 21, 2018

@anba I'm curious about your point about LocalTZA accepting Number and not the more restrictive time value. Why do you feel it should accept values outside of the maximum time value range?

@anba

This comment has been minimized.

Show comment
Hide comment
@anba

anba Mar 21, 2018

Contributor

@apaprocki Because the input can be a local time value which includes the time zone offset, which means it can overflow the maximum allowed time value.

Contributor

anba commented Mar 21, 2018

@apaprocki Because the input can be a local time value which includes the time zone offset, which means it can overflow the maximum allowed time value.

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Apr 4, 2018

Member

@anba Do you believe the specification currently requires such calculations to be done exactly due to the use of mathematical values in all calculations?

Member

littledan commented Apr 4, 2018

@anba Do you believe the specification currently requires such calculations to be done exactly due to the use of mathematical values in all calculations?

@anba

This comment has been minimized.

Show comment
Hide comment
@anba

anba Apr 4, 2018

Contributor

@littledan The difference between mathematical values and IEEE-754 numbers resp. it's safe-integer subset is, for once, not relevant. 😄

I think the test case for that issue was something like this.

// Assume the local time offset is positive, let's say TZ=Europe/Berlin.
var d = new Date(8640000000000000); // d.[[DateValue]] = 8,640,000,000,000,000
d.setMilliseconds(0);

20.3.4.23 Date.prototype.setMilliseconds ( ms )

    1. Let t be LocalTime(? thisTimeValue(this value)).
    2. Let ms be ? ToNumber(ms).
    3. Let time be MakeTime(HourFromTime(t), MinFromTime(t), SecFromTime(t), ms).
    4. Let u be TimeClip(UTC(MakeDate(Day(t), time))).
    5. Set the [[DateValue]] internal slot of this Date object to u.
    6. Return u. 
  • t in step 1 is then 8640000007200000 (8640000000000000 + 2hr TZA for Europe/Berlin).
  • ms in step 2 is 0.
  • time in step 3 is 7200000.
  • Step 4 then invokes UTC(8640000007200000).

20.3.1.9 UTC ( t )

    1. Return t - LocalTZA(t, false). 
  • Calls LocalTZA(8640000007200000, false).
  • But t is not a time value, because it's not in (-8640000000000000, 8640000000000000).

Similar, when new Date(NaN).setMilliseconds(0) is called, it invokes LocalTime and LocalTZA with t = NaN.

Contributor

anba commented Apr 4, 2018

@littledan The difference between mathematical values and IEEE-754 numbers resp. it's safe-integer subset is, for once, not relevant. 😄

I think the test case for that issue was something like this.

// Assume the local time offset is positive, let's say TZ=Europe/Berlin.
var d = new Date(8640000000000000); // d.[[DateValue]] = 8,640,000,000,000,000
d.setMilliseconds(0);

20.3.4.23 Date.prototype.setMilliseconds ( ms )

    1. Let t be LocalTime(? thisTimeValue(this value)).
    2. Let ms be ? ToNumber(ms).
    3. Let time be MakeTime(HourFromTime(t), MinFromTime(t), SecFromTime(t), ms).
    4. Let u be TimeClip(UTC(MakeDate(Day(t), time))).
    5. Set the [[DateValue]] internal slot of this Date object to u.
    6. Return u. 
  • t in step 1 is then 8640000007200000 (8640000000000000 + 2hr TZA for Europe/Berlin).
  • ms in step 2 is 0.
  • time in step 3 is 7200000.
  • Step 4 then invokes UTC(8640000007200000).

20.3.1.9 UTC ( t )

    1. Return t - LocalTZA(t, false). 
  • Calls LocalTZA(8640000007200000, false).
  • But t is not a time value, because it's not in (-8640000000000000, 8640000000000000).

Similar, when new Date(NaN).setMilliseconds(0) is called, it invokes LocalTime and LocalTZA with t = NaN.

@apaprocki

This comment has been minimized.

Show comment
Hide comment
@apaprocki

apaprocki Apr 4, 2018

Contributor

@anba I will incorporate this into my editorial cleanup of LocalTZA.

Contributor

apaprocki commented Apr 4, 2018

@anba I will incorporate this into my editorial cleanup of LocalTZA.

@apaprocki apaprocki referenced a pull request that will close this issue Apr 4, 2018

Open

Editorial: LocalTZA input specification #1160

@apaprocki

This comment has been minimized.

Show comment
Hide comment
@apaprocki

apaprocki Apr 4, 2018

Contributor

@anba Let me know what you think of the text in #1160. Thanks!

Contributor

apaprocki commented Apr 4, 2018

@anba Let me know what you think of the text in #1160. Thanks!

@anba

This comment has been minimized.

Show comment
Hide comment
@anba

anba Apr 12, 2018

Contributor

The text in #1160 looks good to me! 👍

Contributor

anba commented Apr 12, 2018

The text in #1160 looks good to me! 👍

@jungshik

This comment has been minimized.

Show comment
Hide comment
@jungshik

jungshik May 15, 2018

Contributor

The fourth paragraph seems to imply that a daylight saving time offset is always positive. This is no longer true starting with tzdata2018a.

It's dropped but back in tzdata 2018e.

@anba, is your concern about the following ?

at a positive time zone transition (e.g. when the daylight saving time starts or the time zone adjustment is increased due to a time zone rule change)

at a negative time zone transition (e.g. when the daylight saving time ends or the time zone adjustment is decreased due to a time zone rule change)

Contributor

jungshik commented May 15, 2018

The fourth paragraph seems to imply that a daylight saving time offset is always positive. This is no longer true starting with tzdata2018a.

It's dropped but back in tzdata 2018e.

@anba, is your concern about the following ?

at a positive time zone transition (e.g. when the daylight saving time starts or the time zone adjustment is increased due to a time zone rule change)

at a negative time zone transition (e.g. when the daylight saving time ends or the time zone adjustment is decreased due to a time zone rule change)

@apaprocki

This comment has been minimized.

Show comment
Hide comment
@apaprocki
Contributor

apaprocki commented May 15, 2018

@anba

This comment has been minimized.

Show comment
Hide comment
@anba

anba May 16, 2018

Contributor

@jungshik
Yes, that looks like the sentence my nit-picking was about.

Contributor

anba commented May 16, 2018

@jungshik
Yes, that looks like the sentence my nit-picking was about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment