Summary: | [ESNext][BigInt] Implement support for "<" and ">" relational operation | ||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Caio Lima <ticaiolima> | ||||||||||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | Caio Lima <ticaiolima> | ||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | commit-queue, ews-watchlist, fpizlo, jfbastien, keith_miller, mark.lam, msaboff, rmorisset, saam, ticaiolima, webkit-bug-importer, ysuzuki | ||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||
Bug Depends on: | 184474 | ||||||||||||||||||||||||||||||||
Bug Blocks: | 179001, 185929 | ||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Caio Lima
2018-05-07 08:35:15 PDT
Created attachment 339724 [details]
WIP - Patch
This Patch is implementing the basic idea of this Patch, but it is not Spec compliant yet.
Created attachment 339808 [details] WIP - Patch Now Patch is spec compliant, but it depends on https://bugs.webkit.org/show_bug.cgi?id=184474 to compile. Created attachment 340317 [details]
Patch
(In reply to Caio Lima from comment #3) > Created attachment 340317 [details] > Patch Performance comparison is coming soon. Created attachment 340392 [details]
Patch
Comment on attachment 340392 [details] Patch Attachment 340392 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7694760 New failing tests: http/tests/security/canvas-remote-read-remote-video-redirect.html Created attachment 340467 [details]
Archive of layout-test-results from ews206 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 340392 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340392&action=review > Source/JavaScriptCore/runtime/JSBigInt.cpp:526 > +JSBigInt::ComparisonResult JSBigInt::unequalSign(bool leftNegative) I am not convinced that unequalSign, absoluteGreater and absoluteLess are useful to have as methods. Each is a single expression that fits on a single line, is only used once by JSBigInt::compare, does not really make sense outside of that context, and don't have a name that make their semantics especially clear without reading their code. I would just inline them in compare. > Source/JavaScriptCore/runtime/JSBigInt.h:87 > + enum ComparisonResult { We usually prefer "enum class" to "enum", as it prevents accidental type conversions. > JSTests/stress/big-int-less-than-general.js:72 > + I would probably add a test or two where a BigInt is compared to a string that does not represent a number (which should return undefined if I read 4.m.ii and 4.n.ii correctly in https://tc39.github.io/proposal-bigint/#sec-abstract-relational-comparison) > JSTests/stress/big-int-less-than-general.js:111 > +assert(0n < NaN, false, "0n < NaN"); This surprises me: point 4.s. from https://tc39.github.io/proposal-bigint/#sec-abstract-relational-comparison seems to suggest undefined as a result. Comment on attachment 340392 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340392&action=review Thank you for the review and sorry for the late reply here. >> Source/JavaScriptCore/runtime/JSBigInt.h:87 >> + enum ComparisonResult { > > We usually prefer "enum class" to "enum", as it prevents accidental type conversions. Oops. Changed. >> JSTests/stress/big-int-less-than-general.js:72 >> + > > I would probably add a test or two where a BigInt is compared to a string that does not represent a number (which should return undefined if I read 4.m.ii and 4.n.ii correctly in https://tc39.github.io/proposal-bigint/#sec-abstract-relational-comparison) You are right. I'm going to add them. Comparison operations just return booleans. Take a look at https://tc39.github.io/ecma262/#sec-relational-operators-runtime-semantics-evaluation. >> JSTests/stress/big-int-less-than-general.js:111 >> +assert(0n < NaN, false, "0n < NaN"); > > This surprises me: point 4.s. from https://tc39.github.io/proposal-bigint/#sec-abstract-relational-comparison seems to suggest undefined as a result. It is a good observation! However comparison operators only returns booleans. In the case of Abstract Relational Comparison returns undefined, the "<" operation returns "false". https://tc39.github.io/ecma262/#sec-relational-operators-runtime-semantics-evaluation step 7. Created attachment 340791 [details]
Patch
Comment on attachment 340791 [details] Patch Attachment 340791 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/7740754 New failing tests: stress/big-int-greater-than-general.js.big-int-enabled stress/big-int-greater-than-wrapped-values.js.big-int-enabled stress/big-int-less-than-jit.js.big-int-enabled stress/big-int-less-than-order-of-evaluation.js.big-int-enabled stress/big-int-less-than-wrapped-values.js.big-int-enabled stress/big-int-less-than-general.js.big-int-enabled Created attachment 340792 [details]
Patch
Attachment 340792 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/runtime/Operations.h:181: One line control clauses should not use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:583: More than one command on the same line [whitespace/newline] [4]
Total errors found: 2 in 14 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 340794 [details]
Patch
Comment on attachment 340794 [details] Patch Attachment 340794 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/7744784 New failing tests: stress/big-int-greater-than-general.js.big-int-enabled stress/big-int-less-than-general.js.big-int-enabled Created attachment 340809 [details]
Patch
Comment on attachment 340809 [details] Patch Attachment 340809 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/7747294 New failing tests: stress/big-int-greater-than-general.js.big-int-enabled Created attachment 340818 [details]
Patch
> >> JSTests/stress/big-int-less-than-general.js:111
> >> +assert(0n < NaN, false, "0n < NaN");
> >
> > This surprises me: point 4.s. from https://tc39.github.io/proposal-bigint/#sec-abstract-relational-comparison seems to suggest undefined as a result.
>
> It is a good observation! However comparison operators only returns
> booleans. In the case of Abstract Relational Comparison returns undefined,
> the "<" operation returns "false".
> https://tc39.github.io/ecma262/#sec-relational-operators-runtime-semantics-
> evaluation step 7.
Thanks, I had completely missed that part.
Created attachment 341164 [details]
Patch
Comment on attachment 341164 [details] Patch Attachment 341164 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7785476 New failing tests: http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html Created attachment 341178 [details]
Archive of layout-test-results from ews206 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 341164 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341164&action=review r=me with nit. > Source/JavaScriptCore/runtime/Operations.h:262 > + bool result = bigIntCompareLess(callFrame, p1, p2); > + RETURN_IF_EXCEPTION(scope, false); > + return result; Use, scope.release(); return bigIntCompareLess(callFrame, p1, p2); Created attachment 341504 [details]
Patch for Landing
Thank you for the review! Created attachment 341508 [details]
Benchmark Report
This patch is perf neutral
Comment on attachment 341504 [details] Patch for Landing Clearing flags on attachment: 341504 Committed r232273: <https://trac.webkit.org/changeset/232273> All reviewed patches have been landed. Closing bug. |