1300825 - Various jsdate toString clean-ups

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[André Bargull [:anba]]

A few simple clean-ups, separated from the actual bug fixing work.

Comment 1

[André Bargull [:anba]]

I've split the changes into multiple commits to facilitate the review, the changes themselves are relatively simple.

Comment 2

[André Bargull [:anba]]

Attachment: update-comments.patch

Update comments:
- PRMJTime uses 32-bit years, not 16-bit years
- Use "time zone" consistently
- new_explode is also used for date_format
- Remove unnecessary comments like /* helper function */
- Reformat comments (doesn't yet use correct 79 char column limit because the commits were reordered)

Comment 3

[André Bargull [:anba]]

Attachment: rename-date-vars.patch

Change method/variable names:
- Use "localTime" resp. "utcTime" to denote the time type
- Change "new_explode" to "ToPRMJTime", loosely based on chrono::system_clock::to_time_t (except for snake_case <-> camelCase difference)
- Change "date_format" to "FormatDate" to match current conventions

Comment 4

[André Bargull [:anba]]

Attachment: more-sprintfliteral.patch

- Inline print_gmt_string, print_iso_string, print_iso_extended_string (previously separate methods because older versions used function pointers to access these methods)
- Switch snprintf to SprintfLiteral where applicable
- Don't call snprintf/SprintfLiteral for the invalid date string
- Directly call NewStringCopyZ instead of the JSAPI method JS_NewStringCopyZ

Comment 5

[André Bargull [:anba]]

Attachment: other-cleanup.patch

- Move variables to use sites
- Return PRMJTime from ToPRMJTime, return value optimization from compilers will ensure this doesn't lead to extra copying
- Change formatspec to enum class
- Avoid unnecessary call to AdjustTime
- Directly use return value from PRMJ_FormatTime instead of calling strlen

Comment 6

[André Bargull [:anba]]

Attachment: tzinfo-todatestring.patch

- Only compute offset and time zone name when actually needed
- Improves performance of Date.prototype.toString by 8.5x

Comment 7

[Till Schneidereit [:till]]

Comment on attachment 8788532 [details] [diff] [review]
update-comments.patch

Review of attachment 8788532 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, assuming my guess below is right. If not, please at least file a bug about the "XXX" and mention the bug number.

::: js/src/jsdate.cpp
@@ +2569,5 @@
>      split->tm_wday = int8_t(WeekDay(timeval));
>      split->tm_year = year;
>      split->tm_yday = int16_t(DayWithinYear(timeval, year));
>  
> +    // XXX: DaylightSavingTA expects utc-time argument.

I assume this is getting fixed in a consecutive, non-cleanup patch?

Comment 8

[Till Schneidereit [:till]]

Comment on attachment 8788533 [details] [diff] [review]
rename-date-vars.patch

Review of attachment 8788533 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 9

[Till Schneidereit [:till]]

Comment on attachment 8788534 [details] [diff] [review]
more-sprintfliteral.patch

Review of attachment 8788534 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 10

[Till Schneidereit [:till]]

Comment on attachment 8788535 [details] [diff] [review]
other-cleanup.patch

Review of attachment 8788535 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with or without nits addressed.

::: js/src/jsdate.cpp
@@ +2612,5 @@
>              usetz = true;
> +            for (size_t i = 0; i < tzlen; i++) {
> +                char16_t c = tzbuf[i];
> +                if (c > 127 || !(isalnum(c) || c == ' ' || c == '(' || c == ')' || c == '.'))
> +                    usetz = false;

Any reason not to break here?

@@ +2618,5 @@
>  
>              /* Also reject it if it's not parenthesized or if it's '()'. */
>              if (tzbuf[0] != '(' || tzbuf[1] == ')')
>                  usetz = false;
>          } else

pre-existing nit: need braces around the else block because of multi-line then block. (Feel free to ignore, though.)

@@ +2693,5 @@
>              !isdigit(buf[result_len - 3]) &&
>              isdigit(buf[result_len - 2]) && isdigit(buf[result_len - 1]) &&
>              /* ...but not if starts with 4-digit year, like 2022/3/11. */
>              !(isdigit(buf[0]) && isdigit(buf[1]) &&
>                isdigit(buf[2]) && isdigit(buf[3]))) {

Pre-existing nit: opening brace should be on its own line after multi-line conditions.

Comment 11

[Till Schneidereit [:till]]

Comment on attachment 8788536 [details] [diff] [review]
tzinfo-todatestring.patch

Review of attachment 8788536 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Thank you, this patch series makes things quite a bit nicer.

Comment 12

[André Bargull [:anba]]

Attachment: bug1300825-part3.patch

Patch needed bit-unrotting, carrying r+ from till.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d55e89b2945

Comment 13

[André Bargull [:anba]]

Attachment: bug1300825-part4.patch

Patch needed bit-unrotting, carrying r+ from Waldo.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d55e89b2945

Comment 14

[André Bargull [:anba]]

Attachment: bug1300825-part5.patch

Patch needed bit-unrotting, carrying r+ from till.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d55e89b2945

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/632091ca724b
Update comments in jsdate.cpp. r=till
https://hg.mozilla.org/integration/mozilla-inbound/rev/291a6f2dc42e
Rename variables and methods in Date for clarity. r=till
https://hg.mozilla.org/integration/mozilla-inbound/rev/0512a9e149d6
Use more SprintfLiteral in jsdate. r=till
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9fadd6ed3c1
Code clean-ups for date formatting. r=till
https://hg.mozilla.org/integration/mozilla-inbound/rev/67b189765ba9
Don't compute time zone info for Date.prototype.toDateString. r=till

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/632091ca724b
https://hg.mozilla.org/mozilla-central/rev/291a6f2dc42e
https://hg.mozilla.org/mozilla-central/rev/0512a9e149d6
https://hg.mozilla.org/mozilla-central/rev/f9fadd6ed3c1
https://hg.mozilla.org/mozilla-central/rev/67b189765ba9

Comment 17

[Gerry Chang [:gchang]]

Mark 51 as won't fix. If it's worth uplifting to 51, feel free to nominate it.