...
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.
<rdar://problem/40628352>