Improve `Date.parse` #289

chqrlie · 2024-03-03T01:09:18Z

Improve Date.parse

rewrite Date.parse() with separate parsers for ISO and other formats
return NaN for out of bounds field values as specified
accept up to 9 decimals for millisecond fraction but truncate at 3
accept many more alternative date/time formats
add test cases in tests/test_builtin.js
fix most v8 test cases

- produce readable output for Date objects in repl

- rewrite Date.parse() with separate parsers - return `NaN` for out of bounds field values as specified - accept up to 9 decimals for millisecond fraction but truncate at 3 - accept many more alternative date/time formats - add test cases in tests/test_builtin.js

bnoordhuis

Mostly LGTM but I believe the spec allows for some implementation-dependentness and we should decide whether we want to align with V8 (laxer) or SM (stricter.)

Although I don't generally subscribe to Postel's maxim, I'm inclined to side with V8 here because date strings are often entered by humans, not machines.

bnoordhuis · 2024-03-04T09:07:24Z

repl.js

@@ -938,6 +938,8 @@ import * as os from "os";
                    std.puts(a);
                } else if (stack.indexOf(a) >= 0) {
                    std.puts("[circular]");
+                } else if (a instanceof Date) {
+                    std.puts("Date " + a.toGMTString().__quote());


Hm, in my never-ceasing purge of everything and anything non-standard, this method had so far escaped my notice.

How about a toSource, which albeit non-standard is available on some systems and less surprising. For the Date object output, I suggest std.puts(`Date "${a.toGMTString()}"`);

Why not format the string here, in place? I'm not a huge fan of swapping one non-standard method for another. toSource() was a Mozilla-ism, as far as I'm aware, and even they removed it.

edit: doesn't need to hold up this pull request since __quote is a pre-existing thing.

Why not format the string here, in place?

I used std.puts(`Date "${a.toGMTString()}"`); in the latest commit. Is this what you meant?
How to you produce the source for a string reliably without some kind of toSource() method?
I guess I could write it in JS and add it in repl.js or another package loaded by repl.js.

That's what I mean, doing it inside repl.js.

I implemented it in Javascript, then as I proceeded to remove String.prototype.__quote, I realized JSON.stringify does the job. Patch submitted.

quickjs.c

bnoordhuis · 2024-03-04T09:34:40Z

quickjs.c

+}
+
+/* parse toISOString format */
+static BOOL js_date_parse_isostring(const uint8_t *sp, int fields[9], BOOL *is_local) {


Suggested change

static BOOL js_date_parse_isostring(const uint8_t *sp, int fields[9], BOOL *is_local) {

static BOOL js_date_parse_isostring(const uint8_t *sp, int (*fields)[9], BOOL *is_local) {

That way callers must pass in an exactly nine-element array (and you have to dereference them here as (*fields)[i])

Another alternative is C99-style int fields[static 9] syntax. Then callers must pass in an at least nine-element array.

Good suggestion. I don't like the C99 syntax too much. Does MSAN or ASAN catch out of bound accesses for arrays passed as pointers (without a length specified by either syntax)?

Most of the time. Not 100% of the time though.

I added a macro to encapsulate the C99 syntax and used that. It is less surprising to casual readers than the array pointer trick, and also allows for larger arrays if necessary, as is the case for get_date_fields() / set_date_fields() combinations.

I had to disable the C99 syntax for MSVC that does not support it even though it does define __STDC_VERSION__.

The int (*fields)[9] syntax works in msvc; it's a c89-ism that we use quite extensively in libuv.

The int (*fields)[9] syntax works in msvc; it's a c89-ism that we use quite extensively in libuv.

Of course, but this syntax would force the length to be 9 whereas get_date_fields needs at least 9 and set_date_fields at least 7. It is not a problem if MSVC cannot verify the minimum length as other compilers already do for 99,9% of the code.

quickjs.c

bnoordhuis · 2024-03-04T09:36:38Z

quickjs.c

+                return FALSE;
+        }
+    }
+    if (string_skip_char(sp, &p, 'T')) {


Same as my other comment: V8 accepts lowercase 't', spidermonkey doesn't.

T and Z are specified in uppercase for the ISO date/time format. Matching these in lowercase too does not seem necessary, but could become a side effect of the string conversion if I were to uppercase the string unconditionally. I am not sure it is worth it.

I'd personally lean on the side of acceptance and forgiveness here but you're not wrong it's per spec so 🤷

bnoordhuis · 2024-03-04T09:43:45Z

quickjs.c

+        } else
+        if (c == '(') {  /* skip parenthesized phrase */


Suggested change

} else

if (c == '(') { /* skip parenthesized phrase */

} else if (c == '(') { /* skip parenthesized phrase */

Also, I feel slightly offended that this list leaves out CET and CEST :-)

This is a matter of style... I like to align the if tests at the indentation level. I will check if the rest of the code is consistent with that... else if on the same line wins 8 to 1, yet for long lists of chained tests, I find it more readable to break the line between the else and the if.

I would gladly add more timezone abbreviations and use a table. Do V8 or SM support other ones?
I found this source: https://en.wikipedia.org/wiki/List_of_time_zone_abbreviations but it is long and some of the abbreviations are ambiguous.

This site also has a trove of useful information: https://www.timeanddate.com/time/zones/

I wrote a more general parser and added the European zones.

Do V8 or SM support other ones?

Both SM's and V8's parser go well above and beyond the minimum functionality the spec mandates.

They both know how to parse e.g. (Central European Standard Time) (and in a case-insensitive manner); quite possibly they lean into ICU for the actual string parsing.

bnoordhuis · 2024-03-04T09:44:05Z

quickjs.c

+        } else
+        if (c == ')') {


Suggested change

} else

if (c == ')') {

} else if (c == ')') {

- add `minimum_length` macro to specify argument array sizes - improve `string_get_milliseconds` readability - fix `upper_ascii` - add `js_tzabbr` and `string_get_tzabbr` to handle timezone abbreviations - v8.js: parse all environement variables and output them - v8.txt: add environement variables when specified - no longer use `String.prototype.__quote()` to output `Date` objects in repl

- disable C99 `minimum_length()` syntax for MSVC

bnoordhuis · 2024-03-07T23:07:09Z

v8.js

+    let env = {}, envstr = "";
+    for (let s of source.matchAll(/environment variables:(.+)/ig)) {
+        for (let m of s[1].matchAll(/\s*([\S]+)=([\S]+)/g)) {
+            env[m[1]] = m[2];
+            envstr += ` ${m[1]}=${m[2]}`;
+        }
+    }
+    print(`=== ${file}${envstr}`);


Suggested change

let env = {}, envstr = "";

for (let s of source.matchAll(/environment variables:(.+)/ig)) {

for (let m of s[1].matchAll(/\s*([\S]+)=([\S]+)/g)) {

env[m[1]] = m[2];

envstr += ` ${m[1]}=${m[2]}`;

}

}

print(`=== ${file}${envstr}`);

let env = {}, envstr = ""

for (let s of source.matchAll(/environment variables:(.+)/ig)) {

for (let m of s[1].matchAll(/\s*([\S]+)=([\S]+)/g)) {

env[m[1]] = m[2]

envstr += ` ${m[1]}=${m[2]}`

}

}

print(`=== ${file}${envstr}`)

No semicolons in my beautiful pristine JS code, please :-)

OK for consistency, but not my style :)

saghul · 2024-03-09T07:35:51Z

@chqrlie is this ready to go?

I'm planning on cutting a 0.4.0 release once this lands.

chqrlie · 2024-03-09T08:25:43Z

@chqrlie is this ready to go?

I'm planning on cutting a 0.4.0 release once this lands.

Yes. just waiting for Ben's final approval

- use JSON.stringify to output Date and string values in repl.js - remove String.prototype.__quote

bnoordhuis

I only briefly skimmed it this time because, well, it's 10 AM on a Sunday morning and I want to go outside and play with the kids, but LGTM :)

chqrlie added 2 commits March 2, 2024 15:23

Improve Date.parse()

117ddf4

- produce readable output for Date objects in repl

Improve Date.parse

0d302c5

- rewrite Date.parse() with separate parsers - return `NaN` for out of bounds field values as specified - accept up to 9 decimals for millisecond fraction but truncate at 3 - accept many more alternative date/time formats - add test cases in tests/test_builtin.js

chqrlie changed the title ~~Improve date parse~~ Improve Date.parse Mar 3, 2024

bnoordhuis reviewed Mar 4, 2024

View reviewed changes

chqrlie added 2 commits March 5, 2024 22:30

Improve Date.parse()

db0fc07

- disable C99 `minimum_length()` syntax for MSVC

bnoordhuis approved these changes Mar 7, 2024

View reviewed changes

fix style consistency

a9748a3

chqrlie added 3 commits March 9, 2024 13:51

Merge branch 'quickjs-ng:master' into improve-date-parse

992d179

Improve Date.parse

ef69d34

- use JSON.stringify to output Date and string values in repl.js - remove String.prototype.__quote

update gen/repl.c

6ad10ed

bnoordhuis approved these changes Mar 10, 2024

View reviewed changes

chqrlie merged commit 648a8f5 into quickjs-ng:master Mar 10, 2024
36 checks passed

chqrlie deleted the improve-date-parse branch March 10, 2024 11:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `Date.parse` #289

Improve `Date.parse` #289

chqrlie commented Mar 3, 2024

bnoordhuis left a comment

bnoordhuis Mar 4, 2024

chqrlie Mar 4, 2024

bnoordhuis Mar 5, 2024 •

edited

Loading

chqrlie Mar 5, 2024 •

edited

Loading

bnoordhuis Mar 5, 2024

chqrlie Mar 9, 2024 •

edited

Loading

bnoordhuis Mar 4, 2024

chqrlie Mar 4, 2024

bnoordhuis Mar 5, 2024

chqrlie Mar 5, 2024

chqrlie Mar 5, 2024

bnoordhuis Mar 10, 2024

chqrlie Mar 10, 2024

bnoordhuis Mar 4, 2024

chqrlie Mar 4, 2024

bnoordhuis Mar 5, 2024

bnoordhuis Mar 4, 2024

chqrlie Mar 4, 2024 •

edited

Loading

bnoordhuis Mar 5, 2024

bnoordhuis Mar 4, 2024

bnoordhuis Mar 7, 2024

chqrlie Mar 8, 2024

saghul commented Mar 9, 2024

chqrlie commented Mar 9, 2024

bnoordhuis left a comment

	static BOOL js_date_parse_isostring(const uint8_t sp, int fields[9], BOOL is_local) {
	static BOOL js_date_parse_isostring(const uint8_t sp, int (fields)[9], BOOL *is_local) {

	} else
	if (c == '(') { /* skip parenthesized phrase */
	} else if (c == '(') { /* skip parenthesized phrase */

Improve Date.parse #289

Improve Date.parse #289

Conversation

chqrlie commented Mar 3, 2024

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

chqrlie Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chqrlie Mar 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chqrlie Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saghul commented Mar 9, 2024

chqrlie commented Mar 9, 2024

bnoordhuis left a comment

Choose a reason for hiding this comment

Improve `Date.parse` #289

Improve `Date.parse` #289

bnoordhuis Mar 5, 2024 •

edited

Loading

chqrlie Mar 5, 2024 •

edited

Loading

chqrlie Mar 9, 2024 •

edited

Loading

chqrlie Mar 4, 2024 •

edited

Loading