Closed Bug 1300825 Opened 5 years ago Closed 5 years ago

Various jsdate toString clean-ups

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox51 --- wontfix
firefox52 --- fixed

People

(Reporter: anba, Assigned: anba)

References

Details

Attachments

(5 files, 3 obsolete files)

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)
Attachment #8788532 - Flags: review?(till)
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
Attachment #8788533 - Flags: review?(till)
Attached patch more-sprintfliteral.patch (obsolete) — Splinter Review
- 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
Attachment #8788534 - Flags: review?(till)
Attached patch other-cleanup.patch (obsolete) — Splinter Review
- 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
Attachment #8788535 - Flags: review?(till)
Attached patch tzinfo-todatestring.patch (obsolete) — Splinter Review
- Only compute offset and time zone name when actually needed
- Improves performance of Date.prototype.toString by 8.5x
Attachment #8788536 - Flags: review?(till)
Blocks: 830304
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?
Attachment #8788532 - Flags: review?(till) → review+
Comment on attachment 8788533 [details] [diff] [review]
rename-date-vars.patch

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

r=me
Attachment #8788533 - Flags: review?(till) → review+
Comment on attachment 8788534 [details] [diff] [review]
more-sprintfliteral.patch

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

r=me
Attachment #8788534 - Flags: review?(till) → review+
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.
Attachment #8788535 - Flags: review?(till) → review+
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.
Attachment #8788536 - Flags: review?(till) → review+
Patch needed bit-unrotting, carrying r+ from till.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d55e89b2945
Attachment #8788534 - Attachment is obsolete: true
Attachment #8798070 - Flags: review+
Patch needed bit-unrotting, carrying r+ from Waldo.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d55e89b2945
Attachment #8788535 - Attachment is obsolete: true
Attachment #8798071 - Flags: review+
Patch needed bit-unrotting, carrying r+ from till.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d55e89b2945
Attachment #8788536 - Attachment is obsolete: true
Attachment #8798074 - Flags: review+
Keywords: checkin-needed
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
Keywords: checkin-needed
Mark 51 as won't fix. If it's worth uplifting to 51, feel free to nominate it.
You need to log in before you can comment on or make changes to this bug.