RESOLVED FIXED 192615
[BigInt] Simplify boolean context evaluation by leveraging JSString::offsetOfLength() == JSBigInt::offsetOfLength() 
https://bugs.webkit.org/show_bug.cgi?id=192615
Summary [BigInt] Simplify boolean context evaluation by leveraging JSString::offsetOf...
Yusuke Suzuki
Reported 2018-12-11 22:22:36 PST
[BigInt] Simplify boolean context evaluation by leveraging JSString::offsetOfLength() == JSBigInt::offsetOfLength()
Attachments
Patch (12.18 KB, patch)
2018-12-11 22:24 PST, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (12.68 KB, patch)
2018-12-11 22:26 PST, Yusuke Suzuki 		saam: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1 2018-12-11 22:24:43 PST
Created attachment 357105 [details] Patch
Yusuke Suzuki
Comment 2 2018-12-11 22:26:07 PST
Created attachment 357106 [details] Patch
EWS Watchlist
Comment 3 2018-12-11 22:29:39 PST
Attachment 357106 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h:180: JSBigInt_length is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Saam Barati
Comment 4 2018-12-12 00:21:25 PST
Comment on attachment 357106 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357106&action=review > Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.cpp:64 > + , JSBigInt_length(JSString_length) Can we add a new field called something like JSBigIntOrString_length then when we load from this in that scenario we can use that field?
Saam Barati
Comment 5 2018-12-12 00:22:05 PST
Comment on attachment 357106 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357106&action=review > Source/JavaScriptCore/jit/AssemblyHelpers.cpp:799 > + ASSERT(JSString::offsetOfLength() == JSBigInt::offsetOfLength()); Let’s do RELEASE_ASSERT. LLVM will remove this branch anyways
Yusuke Suzuki
Comment 6 2018-12-12 00:28:18 PST
Comment on attachment 357106 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357106&action=review Thank you! >> Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.cpp:64 >> + , JSBigInt_length(JSString_length) > > Can we add a new field called something like JSBigIntOrString_length then when we load from this in that scenario we can use that field? Sounds nice. Fixed. >> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:799 >> + ASSERT(JSString::offsetOfLength() == JSBigInt::offsetOfLength()); > > Let’s do RELEASE_ASSERT. LLVM will remove this branch anyways OK, changed :)
Yusuke Suzuki
Comment 7 2018-12-12 00:35:10 PST
Comment on attachment 357106 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357106&action=review >>> Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.cpp:64 >>> + , JSBigInt_length(JSString_length) >> >> Can we add a new field called something like JSBigIntOrString_length then when we load from this in that scenario we can use that field? > > Sounds nice. Fixed. I added JSBigIntOrString_length field, and make JSString_length and JSBigInt_length as references& to this abstract heap.
Yusuke Suzuki
Comment 8 2018-12-12 00:38:50 PST
Committed r239099: <https://trac.webkit.org/changeset/239099>
Radar WebKit Bug Importer
Comment 9 2018-12-12 00:39:48 PST
<rdar://problem/46654988>
Note You need to log in before you can comment on or make changes to this bug.