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

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

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)

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

- 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

- 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

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

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 on attachment 8788533 [details] [diff] [review] rename-date-vars.patch Review of attachment 8788533 [details] [diff] [review]: ----------------------------------------------------------------- r=me

Comment on attachment 8788534 [details] [diff] [review] more-sprintfliteral.patch Review of attachment 8788534 [details] [diff] [review]: ----------------------------------------------------------------- r=me

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 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.

Patch needed bit-unrotting, carrying r+ from till. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d55e89b2945

Patch needed bit-unrotting, carrying r+ from Waldo. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d55e89b2945

Patch needed bit-unrotting, carrying r+ from till. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d55e89b2945

Pushed by cbook@mozilla.com: 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

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

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