Bug 215839

Summary: [JSC] Implement Intl.DateTimeFormat dayPeriod
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, ews-watchlist, joepeck, keith_miller, mark.lam, msaboff, ross.kirsling, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 213425    
Attachments:
Description Flags
Patch
none
Patch
none
Patch ross.kirsling: review+

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>