WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
185379
[ESNext][BigInt] Implement support for "<" and ">" relational operation
https://bugs.webkit.org/show_bug.cgi?id=185379
Summary
[ESNext][BigInt] Implement support for "<" and ">" relational operation
Caio Lima
Reported
2018-05-07 08:35:15 PDT
...
Attachments
WIP - Patch
(22.03 KB, patch)
2018-05-07 08:39 PDT
,
Caio Lima
no flags
Details
Formatted Diff
Diff
WIP - Patch
(28.83 KB, patch)
2018-05-08 04:50 PDT
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch
(30.23 KB, patch)
2018-05-14 08:16 PDT
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch
(30.48 KB, patch)
2018-05-14 20:19 PDT
,
Caio Lima
ews-watchlist
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from ews206 for win-future
(12.75 MB, application/zip)
2018-05-15 21:45 PDT
,
EWS Watchlist
no flags
Details
Patch
(29.15 KB, patch)
2018-05-19 19:58 PDT
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch
(29.74 KB, patch)
2018-05-19 22:47 PDT
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch
(35.84 KB, patch)
2018-05-20 09:22 PDT
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch
(35.95 KB, patch)
2018-05-20 16:13 PDT
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch
(35.95 KB, patch)
2018-05-20 20:31 PDT
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch
(35.36 KB, patch)
2018-05-23 20:34 PDT
,
Caio Lima
ysuzuki
: review+
ews-watchlist
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from ews206 for win-future
(12.73 MB, application/zip)
2018-05-24 00:25 PDT
,
EWS Watchlist
no flags
Details
Patch for Landing
(35.26 KB, patch)
2018-05-29 12:15 PDT
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Benchmark Report
(93.82 KB, text/plain)
2018-05-29 13:28 PDT
,
Caio Lima
no flags
Details
Show Obsolete
(12)
View All
Add attachment
proposed patch, testcase, etc.
Caio Lima
Comment 1
2018-05-07 08:39:06 PDT
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
Comment 2
2018-05-08 04:50:53 PDT
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
Comment 3
2018-05-14 08:16:57 PDT
Created
attachment 340317
[details]
Patch
Caio Lima
Comment 4
2018-05-14 08:19:00 PDT
(In reply to Caio Lima from
comment #3
)
> Created
attachment 340317
[details]
> Patch
Performance comparison is coming soon.
Caio Lima
Comment 5
2018-05-14 20:19:07 PDT
Created
attachment 340392
[details]
Patch
EWS Watchlist
Comment 6
2018-05-15 21:44:56 PDT
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
Comment 7
2018-05-15 21:45:08 PDT
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 8
2018-05-17 08:23:18 PDT
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 9
2018-05-19 19:53:17 PDT
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
Comment 10
2018-05-19 19:58:03 PDT
Created
attachment 340791
[details]
Patch
EWS Watchlist
Comment 11
2018-05-19 21:18:24 PDT
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
Comment 12
2018-05-19 22:47:32 PDT
Created
attachment 340792
[details]
Patch
EWS Watchlist
Comment 13
2018-05-19 22:49:35 PDT
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
Comment 14
2018-05-20 09:22:09 PDT
Created
attachment 340794
[details]
Patch
EWS Watchlist
Comment 15
2018-05-20 10:40:50 PDT
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
Comment 16
2018-05-20 16:13:41 PDT
Created
attachment 340809
[details]
Patch
EWS Watchlist
Comment 17
2018-05-20 17:34:09 PDT
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
Comment 18
2018-05-20 20:31:17 PDT
Created
attachment 340818
[details]
Patch
Robin Morisset
Comment 19
2018-05-22 03:21:12 PDT
> >> 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
Comment 20
2018-05-23 20:34:25 PDT
Created
attachment 341164
[details]
Patch
EWS Watchlist
Comment 21
2018-05-24 00:24:56 PDT
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
Comment 22
2018-05-24 00:25:07 PDT
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 23
2018-05-29 10:59:41 PDT
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
Comment 24
2018-05-29 12:15:43 PDT
Created
attachment 341504
[details]
Patch for Landing
Caio Lima
Comment 25
2018-05-29 12:21:12 PDT
Thank you for the review!
Caio Lima
Comment 26
2018-05-29 13:28:49 PDT
Created
attachment 341508
[details]
Benchmark Report This patch is perf neutral
WebKit Commit Bot
Comment 27
2018-05-29 14:15:30 PDT
Comment on
attachment 341504
[details]
Patch for Landing Clearing flags on attachment: 341504 Committed
r232273
: <
https://trac.webkit.org/changeset/232273
>
WebKit Commit Bot
Comment 28
2018-05-29 14:15:32 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 29
2018-05-29 14:18:03 PDT
<
rdar://problem/40628352
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug