Bug 215839 - [JSC] Implement Intl.DateTimeFormat dayPeriod
Summary: [JSC] Implement Intl.DateTimeFormat dayPeriod
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks: 213425
  Show dependency tree / graph
 
Reported: 2020-08-25 23:15 PDT by Yusuke Suzuki
Modified: 2020-08-29 01:33 PDT (History)
10 users (show)

See Also:

Attachments
Patch (14.52 KB, patch)
2020-08-25 23:19 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (17.34 KB, patch)
2020-08-28 23:42 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (17.40 KB, patch)
2020-08-28 23:43 PDT, Yusuke Suzuki 		ross.kirsling: review+
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2020-08-25 23:15:44 PDT 
https://github.com/tc39/ecma402/pull/346
Comment 1 Yusuke Suzuki 2020-08-25 23:19:11 PDT 
Created attachment 407275 [details]
Patch
Comment 2 Darin Adler 2020-08-26 15:15:09 PDT 
Comment on attachment 407275 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407275&action=review

> Source/JavaScriptCore/ChangeLog:9
> +        and it shows how "AM" etc. is represented.

"it shows" -> "it determines"

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:633
> +            skeletonBuilder.appendLiteral("B");

Slightly more efficient to append 'B' than "B".

Also, we should get rid of StringBuilder::appendLiteral. For one thing, the syntax for that optimization should just be append("this is a literal"_s). But for another, the "literal" optimization only affects cases where it’s the first thing appended to a string builder. Otherwise, it’s no different from append("this is a literal").
Comment 3 Yusuke Suzuki 2020-08-28 23:22:15 PDT 
Comment on attachment 407275 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407275&action=review

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:633
>> +            skeletonBuilder.appendLiteral("B");
> 
> Slightly more efficient to append 'B' than "B".
> 
> Also, we should get rid of StringBuilder::appendLiteral. For one thing, the syntax for that optimization should just be append("this is a literal"_s). But for another, the "literal" optimization only affects cases where it’s the first thing appended to a string builder. Otherwise, it’s no different from append("this is a literal").

Changed to `append('B')`. For now, using appendLiteral, but I agree. append("...") would be the same because `strlen()` will be constant-folded anyway.
Comment 4 Yusuke Suzuki 2020-08-28 23:42:25 PDT 
Comment on attachment 407275 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407275&action=review

>> Source/JavaScriptCore/ChangeLog:9
>> +        and it shows how "AM" etc. is represented.
> 
> "it shows" -> "it determines"

Fixed.
Comment 5 Yusuke Suzuki 2020-08-28 23:42:37 PDT 
Created attachment 407532 [details]
Patch
Comment 6 Yusuke Suzuki 2020-08-28 23:43:44 PDT 
Created attachment 407533 [details]
Patch
Comment 7 Ross Kirsling 2020-08-29 00:01:45 PDT 
Comment on attachment 407533 [details]
Patch

r=me, though the flag intrinsic makes me super sad.
Comment 8 Yusuke Suzuki 2020-08-29 01:32:50 PDT 
Committed r266323: <https://trac.webkit.org/changeset/266323>
Comment 9 Radar WebKit Bug Importer 2020-08-29 01:33:37 PDT 
<rdar://problem/67985176>