Bug 185379 – [ESNext][BigInt] Implement support for "<" and ">" relational operation

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Caio Lima]

...

[Caio Lima]

Created attachment 339724 [details]
WIP - Patch

This Patch is implementing the basic idea of this Patch, but it is not Spec compliant yet.

[Caio Lima]

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.

[Caio Lima]

Created attachment 340317 [details]
Patch

[Caio Lima]

(In reply to Caio Lima from comment #3)
> Created attachment 340317 [details]
> Patch

Performance comparison is coming soon.

[Caio Lima]

Created attachment 340392 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Robin Morisset]

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.

[Caio Lima]

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.

[Caio Lima]

Created attachment 340791 [details]
Patch

[EWS Watchlist]

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

[Caio Lima]

Created attachment 340792 [details]
Patch

[EWS Watchlist]

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.

[Caio Lima]

Created attachment 340794 [details]
Patch

[EWS Watchlist]

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

[Caio Lima]

Created attachment 340809 [details]
Patch

[EWS Watchlist]

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

[Caio Lima]

Created attachment 340818 [details]
Patch

[Robin Morisset]

> >> 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.

[Caio Lima]

Created attachment 341164 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Yusuke Suzuki]

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);

[Caio Lima]

Created attachment 341504 [details]
Patch for Landing

[Caio Lima]

Thank you for the review!

[Caio Lima]

Created attachment 341508 [details]
Benchmark Report

This patch is perf neutral

[WebKit Commit Bot]

Comment on attachment 341504 [details]
Patch for Landing

Clearing flags on attachment: 341504

Committed r232273: <https://trac.webkit.org/changeset/232273>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40628352>