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

introduce proper LocalTZA(t,isUTC) #239

Closed
drsm opened this issue Oct 13, 2019 · 5 comments
Closed

introduce proper LocalTZA(t,isUTC) #239

drsm opened this issue Oct 13, 2019 · 5 comments

Comments

@drsm
Copy link
Contributor

drsm commented Oct 13, 2019

the spec
full story
v8

Here is the patch:

# HG changeset patch
# User Artem S. Povalyukhin <artem.povaluhin@gmail.com>
# Date 1570924801 -10800
#      Sun Oct 13 03:00:01 2019 +0300
# Node ID 4f6b83afe428575ea60068f23042a6980cbc91f1
# Parent  30c1bde3f194452a42dfc6af35ed260d74218e90
Introduced njs_local_tza().

diff -r 30c1bde3f194 -r 4f6b83afe428 src/njs_date.c
--- a/src/njs_date.c	Fri Oct 11 19:41:51 2019 +0300
+++ b/src/njs_date.c	Sun Oct 13 03:00:01 2019 +0300
@@ -138,8 +138,8 @@ njs_make_day(int64_t yr, int64_t month, 
 }
 
 
-njs_inline int64_t
-njs_tz_offset(int64_t time)
+njs_inline int64_t
+njs_local_tza(int64_t time, njs_bool_t is_utc)
 {
     time_t     ti;
     struct tm  tm;
@@ -149,7 +149,12 @@ njs_tz_offset(int64_t time)
     ti = time;
     localtime_r(&ti, &tm);
 
-    return -njs_timezone(&tm) / 60;
+    if (!is_utc) {
+        ti -= (njs_timezone(&tm) + 3600 * 1000);
+        localtime_r(&ti, &tm);
+    }
+
+    return njs_timezone(&tm) * 1000;
 }
 
 
@@ -161,7 +166,7 @@ njs_make_date(int64_t days, int64_t time
     date = days * 86400000 + time;
 
     if (local) {
-        date += njs_tz_offset(date) * 60000;
+        date -= njs_local_tza(date, 0);
     }
 
     return date;
@@ -1505,7 +1510,7 @@ njs_date_prototype_get_timezone_offset(n
     value = njs_date(&args[0])->time;
 
     if (njs_fast_path(!isnan(value))) {
-        value = njs_tz_offset(value);
+        value = 0.0 - (njs_local_tza((int64_t) value, 1) / 60000);
     }
 
     njs_set_number(&vm->retval, value);
diff -r 30c1bde3f194 -r 4f6b83afe428 src/test/njs_unit_test.c
--- a/src/test/njs_unit_test.c	Fri Oct 11 19:41:51 2019 +0300
+++ b/src/test/njs_unit_test.c	Sun Oct 13 03:00:01 2019 +0300
@@ -14505,6 +14505,24 @@ static njs_unit_test_t  njs_tz_test[] =
      { njs_str("var d = new Date(2011, 5, 24, 18, 45, 12, 625);"
                   "d.toJSON(1)"),
        njs_str("2011-06-24T06:00:12.625Z") },
+
+#if 0
+     { njs_str("var d = new Date(-1, 0);"
+                  "d.toISOString()"),
+       njs_str("-000002-12-31T11:46:12.000Z") },
+#endif
+
+     { njs_str("var d = new Date(-1, 0);"
+                  "d.getTime()"),
+       njs_str("-62198799228000") },
+
+     { njs_str("var d = new Date(-1, 0);"
+                  "d.getTimezoneOffset()"),
+       njs_str("-733") },
+
+     { njs_str("var d = new Date(1970, 6);"
+                  "d.getTimezoneOffset()"),
+       njs_str("-765") },
 };
 
 
@xeioex
Copy link
Contributor

xeioex commented Nov 8, 2019

@drsm See 989d85c#diff-dbf994c61cc8c6d79786ce6801aad8a5R184 why njs_local_tza() does not work as expected. Left njs_tz_offset() as is, added extra tests from your patch.

@drsm
Copy link
Contributor Author

drsm commented Nov 8, 2019

@xeioex

Actually, looks like a bug in test262,
node:

> var args = [2014, 0]
undefined
> var d = new Date(...args); d.valueOf() - d.getTimezoneOffset() * 60000 - Date.UTC(...args);
0
> var args = [-2014, 0]
undefined
> var d = new Date(...args); d.valueOf() - d.getTimezoneOffset() * 60000 - Date.UTC(...args);
-17000
> d.toString()
'Wed Jan 01 -2014 00:00:00 GMT+0230 (Moscow Standard Time)'
>

but, there is still no consensus about this:

root@node:~# TZ='Europe/Moscow' eshost -x 'var d = new Date(-2014, 0); print(d.valueOf() - d.getTimezoneOffset() * 60000 - Date.UTC(-2014, 0))'
#### ch
0

#### jsc
0

#### sm
0

#### v8
-17000

#### xs
0

@xeioex
Copy link
Contributor

xeioex commented Nov 8, 2019

@drsm

I guess, usually there is not such strange timezones as in
zdump -v /etc/localtime
https://gist.github.com/248a93e0126f12755ac29e5e5932043f

/etc/localtime  Wed Dec 31 21:29:42 1879 UT = Wed Dec 31 23:59:59 1879 LMT isdst=0 gmtoff=9017
/etc/localtime  Wed Dec 31 21:29:43 1879 UT = Thu Jan  1 00:00:00 1880 MMT isdst=0 gmtoff=9017
$ TEST=$(find ./test262/ | grep 'built-ins/Date/S15.9.3.1_A5_T1.js');  cat test262/harness/assert.js test262/harness/propertyHelper.js  test262/harness/sta.js test262/harness/assertRelativeDateMs.js $TEST > test.js

$ node -v
v10.16.0
$ node test.js                                                                                                             

/home/xeioex/workspace/nginx/nginScript/test262-harness-py/test.js:359
  throw new Test262Error(message);
  ^
Test262Error: Expected Fri Dec 01 1899 00:00:00 GMT+0230 (Moscow Standard Time) to be -2211667200000 milliseconds from the Unix epoch

# BUT: OK
$ TZ=MSK node test.js

@drsm
Copy link
Contributor Author

drsm commented Nov 8, 2019

@xeioex

The fix introduced a "weird seconds problem". But it's still not covered by test262.
And harness/assertRelativeDateMs.js conflicts with the current spec.

I'm OK with the current implementation, as the proper one, which works well in every corner case, will require an ICU stuff as a dependency.

# BUT: OK
$ TZ=MSK node test.js

looks like it didn't understand MSK:

$ TZ=MSK node -e 'console.log((new Date()).toString())'
Fri Nov 08 2019 20:51:03 GMT+0000 (GMT)
$ TZ=Europe/Moscow node -e 'console.log((new Date()).toString())'
Fri Nov 08 2019 23:51:15 GMT+0300 (Moscow Standard Time)

@xeioex
Copy link
Contributor

xeioex commented Nov 8, 2019

@drsm

OK, I think we simply can wait until test262 is updated.

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

No branches or pull requests

2 participants