WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
179002
[ESNext][BigInt] Implement support for addition operations
https://bugs.webkit.org/show_bug.cgi?id=179002
Summary
[ESNext][BigInt] Implement support for addition operations
Caio Lima
Reported
2017-10-30 04:03:20 PDT
Implement here support for additional arithmetic operations "+" and "-"
Attachments
Patch
(71.72 KB, patch)
2018-01-21 15:56 PST
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews102 for mac-sierra
(479.36 KB, application/zip)
2018-01-21 16:51 PST
,
EWS Watchlist
no flags
Details
Archive of layout-test-results from ews107 for mac-sierra-wk2
(889.62 KB, application/zip)
2018-01-21 16:59 PST
,
EWS Watchlist
no flags
Details
Archive of layout-test-results from ews112 for mac-sierra
(1.01 MB, application/zip)
2018-01-21 17:09 PST
,
EWS Watchlist
no flags
Details
Archive of layout-test-results from ews124 for ios-simulator-wk2
(501.46 KB, application/zip)
2018-01-21 17:16 PST
,
EWS Watchlist
no flags
Details
Patch
(73.00 KB, patch)
2018-01-23 14:37 PST
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch
(73.00 KB, patch)
2018-01-23 15:26 PST
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews102 for mac-sierra
(2.20 MB, application/zip)
2018-01-23 16:40 PST
,
EWS Watchlist
no flags
Details
Archive of layout-test-results from ews107 for mac-sierra-wk2
(3.37 MB, application/zip)
2018-01-23 16:41 PST
,
EWS Watchlist
no flags
Details
Archive of layout-test-results from ews126 for ios-simulator-wk2
(2.17 MB, application/zip)
2018-01-23 17:08 PST
,
EWS Watchlist
no flags
Details
Archive of layout-test-results from ews112 for mac-sierra
(3.16 MB, application/zip)
2018-01-23 17:26 PST
,
EWS Watchlist
no flags
Details
Patch
(70.55 KB, patch)
2018-01-24 02:48 PST
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews104 for mac-sierra-wk2
(2.55 MB, application/zip)
2018-01-24 04:03 PST
,
EWS Watchlist
no flags
Details
Patch
(70.54 KB, patch)
2018-01-24 14:04 PST
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch
(73.25 KB, patch)
2018-01-25 14:31 PST
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch
(73.23 KB, patch)
2018-01-25 14:44 PST
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Benchmark Reports
(87.65 KB, text/plain)
2018-01-25 14:46 PST
,
Caio Lima
no flags
Details
Patch
(60.80 KB, patch)
2018-05-31 20:00 PDT
,
Caio Lima
ysuzuki
: review+
Details
Formatted Diff
Diff
Benchmark Report
(93.41 KB, text/plain)
2018-06-01 07:00 PDT
,
Caio Lima
no flags
Details
Patch for landing
(69.52 KB, patch)
2018-06-01 19:22 PDT
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch for landing
(68.05 KB, patch)
2018-06-02 18:41 PDT
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Show Obsolete
(19)
View All
Add attachment
proposed patch, testcase, etc.
Caio Lima
Comment 1
2018-01-21 15:56:23 PST
Created
attachment 331884
[details]
Patch
EWS Watchlist
Comment 2
2018-01-21 15:58:49 PST
Attachment 331884
[details]
did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:78: The parameter name "sign" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:129: The parameter name "sign" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:129: The parameter name "parseIntMode" adds no information, so it should be removed. [readability/parameter_name] [5] WARNING: This machine could support 4 simulators, but is only configured for 3. WARNING: Please see <
https://trac.webkit.org/wiki/IncreasingKernelLimits
>. Total errors found: 3 in 30 files If any of these errors are false positives, please file a bug against check-webkit-style.
EWS Watchlist
Comment 3
2018-01-21 16:51:01 PST
Comment on
attachment 331884
[details]
Patch
Attachment 331884
[details]
did not pass mac-ews (mac): Output:
http://webkit-queues.webkit.org/results/6162674
Number of test failures exceeded the failure limit.
EWS Watchlist
Comment 4
2018-01-21 16:51:02 PST
Created
attachment 331885
[details]
Archive of layout-test-results from ews102 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
EWS Watchlist
Comment 5
2018-01-21 16:59:39 PST
Comment on
attachment 331884
[details]
Patch
Attachment 331884
[details]
did not pass mac-wk2-ews (mac-wk2): Output:
http://webkit-queues.webkit.org/results/6162682
Number of test failures exceeded the failure limit.
EWS Watchlist
Comment 6
2018-01-21 16:59:40 PST
Created
attachment 331886
[details]
Archive of layout-test-results from ews107 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
EWS Watchlist
Comment 7
2018-01-21 17:09:10 PST
Comment on
attachment 331884
[details]
Patch
Attachment 331884
[details]
did not pass mac-debug-ews (mac): Output:
http://webkit-queues.webkit.org/results/6162688
Number of test failures exceeded the failure limit.
EWS Watchlist
Comment 8
2018-01-21 17:09:11 PST
Created
attachment 331887
[details]
Archive of layout-test-results from ews112 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-sierra Platform: Mac OS X 10.12.6
EWS Watchlist
Comment 9
2018-01-21 17:16:53 PST
Comment on
attachment 331884
[details]
Patch
Attachment 331884
[details]
did not pass ios-sim-ews (ios-simulator-wk2): Output:
http://webkit-queues.webkit.org/results/6162718
Number of test failures exceeded the failure limit.
EWS Watchlist
Comment 10
2018-01-21 17:16:54 PST
Created
attachment 331888
[details]
Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Caio Lima
Comment 11
2018-01-23 14:37:19 PST
Created
attachment 332079
[details]
Patch
Caio Lima
Comment 12
2018-01-23 15:26:23 PST
Created
attachment 332085
[details]
Patch
EWS Watchlist
Comment 13
2018-01-23 16:40:13 PST
Comment on
attachment 332085
[details]
Patch
Attachment 332085
[details]
did not pass jsc-ews (mac): Output:
http://webkit-queues.webkit.org/results/6187861
New failing tests: mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-llint mozilla-tests.yaml/ecma/Date/15.9.2.2-2.js.mozilla-baseline mozilla-tests.yaml/ecma/Date/15.9.2.2-3.js.mozilla-no-ftl mozilla-tests.yaml/ecma/Date/15.9.2.2-4.js.mozilla-ftl-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma/Date/15.9.2.2-4.js.mozilla mozilla-tests.yaml/ecma/Date/15.9.2.2-3.js.mozilla mozilla-tests.yaml/ecma/Date/15.9.2.1.js.mozilla-dfg-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma/Array/15.4.4.5-3.js.mozilla-dfg-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma/Date/15.9.2.2-4.js.mozilla-no-ftl mozilla-tests.yaml/ecma/Date/15.9.2.2-4.js.mozilla-baseline mozilla-tests.yaml/ecma/Date/15.9.2.1.js.mozilla-ftl-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma/Date/15.9.2.2-3.js.mozilla-ftl-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma/Date/15.9.2.2-1.js.mozilla-dfg-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma/Date/15.9.2.1.js.mozilla-no-ftl mozilla-tests.yaml/ecma/Array/15.4.4.5-3.js.mozilla-llint mozilla-tests.yaml/ecma/Array/15.4.4.5-3.js.mozilla-no-ftl mozilla-tests.yaml/ecma_3/RegExp/regress-85721.js.mozilla-ftl-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma_3/RegExp/regress-85721.js.mozilla-dfg-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma/Array/15.4.4.5-3.js.mozilla-ftl-eager-no-cjit-validate-phases mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla mozilla-tests.yaml/ecma/Date/15.9.2.1.js.mozilla-baseline mozilla-tests.yaml/ecma_3/RegExp/regress-85721.js.mozilla-llint mozilla-tests.yaml/ecma/Date/15.9.2.2-2.js.mozilla-ftl-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma/Date/15.9.2.2-3.js.mozilla-baseline mozilla-tests.yaml/ecma/Date/15.9.2.2-6.js.mozilla-baseline mozilla-tests.yaml/ecma_3/RegExp/regress-85721.js.mozilla mozilla-tests.yaml/ecma/Date/15.9.2.2-5.js.mozilla-dfg-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma/Date/15.9.2.2-5.js.mozilla mozilla-tests.yaml/ecma/Date/15.9.2.2-5.js.mozilla-baseline mozilla-tests.yaml/ecma/Date/15.9.2.2-1.js.mozilla-llint mozilla-tests.yaml/ecma/Date/15.9.2.2-2.js.mozilla-no-ftl mozilla-tests.yaml/ecma/Date/15.9.2.2-1.js.mozilla mozilla-tests.yaml/ecma/Date/15.9.2.2-4.js.mozilla-llint mozilla-tests.yaml/ecma/Date/15.9.2.2-5.js.mozilla-llint mozilla-tests.yaml/ecma/Date/15.9.2.2-1.js.mozilla-ftl-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma/Date/15.9.2.2-1.js.mozilla-no-ftl mozilla-tests.yaml/ecma/Date/15.9.2.1.js.mozilla-llint mozilla-tests.yaml/ecma/Date/15.9.2.2-3.js.mozilla-dfg-eager-no-cjit-validate-phases mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-no-ftl mozilla-tests.yaml/ecma_3/RegExp/regress-85721.js.mozilla-baseline mozilla-tests.yaml/ecma/Date/15.9.2.2-4.js.mozilla-dfg-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma/Date/15.9.2.1.js.mozilla mozilla-tests.yaml/ecma_3/RegExp/regress-85721.js.mozilla-no-ftl mozilla-tests.yaml/ecma/Array/15.4.4.5-3.js.mozilla-baseline mozilla-tests.yaml/ecma/Date/15.9.2.2-1.js.mozilla-baseline mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-ftl-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma/Date/15.9.2.2-2.js.mozilla-llint mozilla-tests.yaml/ecma/Date/15.9.2.2-6.js.mozilla-no-ftl mozilla-tests.yaml/ecma/Date/15.9.2.2-5.js.mozilla-no-ftl mozilla-tests.yaml/ecma/Date/15.9.2.2-6.js.mozilla-llint mozilla-tests.yaml/ecma/Date/15.9.2.2-6.js.mozilla-ftl-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma/Date/15.9.2.2-6.js.mozilla mozilla-tests.yaml/ecma/Array/15.4.4.5-3.js.mozilla mozilla-tests.yaml/ecma/Date/15.9.2.2-2.js.mozilla-dfg-eager-no-cjit-validate-phases mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-baseline mozilla-tests.yaml/ecma/Date/15.9.2.2-5.js.mozilla-ftl-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma/Date/15.9.2.2-3.js.mozilla-llint mozilla-tests.yaml/ecma/Date/15.9.2.2-6.js.mozilla-dfg-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma/Date/15.9.2.2-2.js.mozilla mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-dfg-eager-no-cjit-validate-phases
EWS Watchlist
Comment 14
2018-01-23 16:40:49 PST
Comment on
attachment 332085
[details]
Patch
Attachment 332085
[details]
did not pass mac-ews (mac): Output:
http://webkit-queues.webkit.org/results/6187885
New failing tests: http/tests/misc/no-last-modified.html imported/w3c/IndexedDB-private-browsing/keyorder.html imported/w3c/web-platform-tests/IndexedDB/keyorder.htm imported/w3c/web-platform-tests/user-timing/measure_navigation_timing.html imported/w3c/web-platform-tests/user-timing/measure.html media/video-controller-child-rate.html
EWS Watchlist
Comment 15
2018-01-23 16:40:50 PST
Created
attachment 332096
[details]
Archive of layout-test-results from ews102 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
EWS Watchlist
Comment 16
2018-01-23 16:41:39 PST
Comment on
attachment 332085
[details]
Patch
Attachment 332085
[details]
did not pass mac-wk2-ews (mac-wk2): Output:
http://webkit-queues.webkit.org/results/6187870
New failing tests: http/tests/misc/no-last-modified.html imported/w3c/IndexedDB-private-browsing/keyorder.html imported/w3c/web-platform-tests/user-timing/measure.html imported/w3c/web-platform-tests/user-timing/measure_navigation_timing.html imported/w3c/web-platform-tests/IndexedDB/keyorder.htm media/video-controller-child-rate.html
EWS Watchlist
Comment 17
2018-01-23 16:41:41 PST
Created
attachment 332097
[details]
Archive of layout-test-results from ews107 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
EWS Watchlist
Comment 18
2018-01-23 17:08:11 PST
Comment on
attachment 332085
[details]
Patch
Attachment 332085
[details]
did not pass ios-sim-ews (ios-simulator-wk2): Output:
http://webkit-queues.webkit.org/results/6187973
New failing tests: http/tests/misc/no-last-modified.html imported/w3c/IndexedDB-private-browsing/keyorder.html imported/w3c/web-platform-tests/IndexedDB/keyorder.htm imported/w3c/web-platform-tests/user-timing/measure_navigation_timing.html imported/w3c/web-platform-tests/user-timing/measure.html media/video-controller-child-rate.html
EWS Watchlist
Comment 19
2018-01-23 17:08:12 PST
Created
attachment 332101
[details]
Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
EWS Watchlist
Comment 20
2018-01-23 17:26:46 PST
Comment on
attachment 332085
[details]
Patch
Attachment 332085
[details]
did not pass mac-debug-ews (mac): Output:
http://webkit-queues.webkit.org/results/6188005
New failing tests: http/tests/misc/no-last-modified.html imported/w3c/IndexedDB-private-browsing/keyorder.html imported/w3c/web-platform-tests/user-timing/measure.html imported/w3c/web-platform-tests/user-timing/measure_navigation_timing.html imported/w3c/web-platform-tests/IndexedDB/keyorder.htm media/video-controller-child-rate.html
EWS Watchlist
Comment 21
2018-01-23 17:26:47 PST
Created
attachment 332106
[details]
Archive of layout-test-results from ews112 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-sierra Platform: Mac OS X 10.12.6
Caio Lima
Comment 22
2018-01-24 02:48:01 PST
Created
attachment 332144
[details]
Patch
EWS Watchlist
Comment 23
2018-01-24 04:03:23 PST
Comment on
attachment 332144
[details]
Patch
Attachment 332144
[details]
did not pass mac-wk2-ews (mac-wk2): Output:
http://webkit-queues.webkit.org/results/6193545
New failing tests: fast/workers/worker-cloneport.html
EWS Watchlist
Comment 24
2018-01-24 04:03:25 PST
Created
attachment 332146
[details]
Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Caio Lima
Comment 25
2018-01-24 14:04:46 PST
Created
attachment 332195
[details]
Patch
Joseph Pecoraro
Comment 26
2018-01-25 12:03:46 PST
Comment on
attachment 332195
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=332195&action=review
These tests are great! I'd like to see a few more edge cases: • BigInt [+/-] with exotic values like NaN / Infinity: 1n + Infinity; Infinity + 1n; 1n + NaN; NaN + 1n; • BigInt [+/-] with other types / falsey values like null / undefined / bool / empty string: 1n + null; 1n + undefined; 1n + true; 1n + false; 1n + ""; • Did I read the ChangeLog correctly that unary minus isn't supported just yet? If it is we could add tests like: x = 1n; y = -x; z = -y; x === z;
> JSTests/stress/big-int-addition-type-error.js:17 > +assertThrowTypeError(30n, Symbol("foo"), "BingInt + Symbol");
Typos: "BingInt" in this file should be "BigInt"
> JSTests/stress/big-int-subtraction-basic.js:39 > +testSub(0xFEDCBA987654320Fn, 0x0n, 0xFEDCBA987654320Fn); testSub(0xFEDCBA987654320Fn, -0x1n, 0xFEDCBA9876543210n);
Style: Probably want to split these up into two lines.
Caio Lima
Comment 27
2018-01-25 13:31:50 PST
(In reply to Joseph Pecoraro from
comment #26
)
> Comment on
attachment 332195
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=332195&action=review
> > These tests are great! I'd like to see a few more edge cases: > > • BigInt [+/-] with exotic values like NaN / Infinity: > 1n + Infinity; Infinity + 1n; > 1n + NaN; NaN + 1n; > > • BigInt [+/-] with other types / falsey values like null / undefined / bool > / empty string: > 1n + null; > 1n + undefined; > 1n + true; > 1n + false; > 1n + "";
Thank you very much to remember these cases!
> • Did I read the ChangeLog correctly that unary minus isn't supported just > yet? If it is we could add tests like: > x = 1n; y = -x; z = -y; x === z;
No. I just implemented the -[BigInt literal] support into AST. I'm not liking it so much, so probably I would make this Bug dependent on unary operations. I'm going to start work on them today.
> > JSTests/stress/big-int-addition-type-error.js:17 > > +assertThrowTypeError(30n, Symbol("foo"), "BingInt + Symbol"); > > Typos: "BingInt" in this file should be "BigInt"
Oops.
> > JSTests/stress/big-int-subtraction-basic.js:39 > > +testSub(0xFEDCBA987654320Fn, 0x0n, 0xFEDCBA987654320Fn); testSub(0xFEDCBA987654320Fn, -0x1n, 0xFEDCBA9876543210n); > > Style: Probably want to split these up into two lines.
Oops.
Caio Lima
Comment 28
2018-01-25 14:31:37 PST
Created
attachment 332317
[details]
Patch
Caio Lima
Comment 29
2018-01-25 14:44:26 PST
Created
attachment 332318
[details]
Patch
Caio Lima
Comment 30
2018-01-25 14:46:54 PST
Created
attachment 332320
[details]
Benchmark Reports This is the run comparing possible overhead of BigInt support into "+" and "-" operations. The benchmarks are neutral.
Joseph Pecoraro
Comment 31
2018-01-29 11:17:25 PST
Comment on
attachment 332318
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=332318&action=review
> JSTests/stress/big-int-subtraction-type-error.js:17 > +assertThrowTypeError(30n, Symbol("foo"), "BingInt - Symbol");
Nice! A few more "BingInt"s to clean up in this file.
Caio Lima
Comment 32
2018-05-31 20:00:21 PDT
Created
attachment 341725
[details]
Patch
Caio Lima
Comment 33
2018-06-01 07:00:43 PDT
Created
attachment 341759
[details]
Benchmark Report This patch is perf neutral.
Yusuke Suzuki
Comment 34
2018-06-01 08:43:28 PDT
Comment on
attachment 341725
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=341725&action=review
r=me with nits.
> Source/JavaScriptCore/ChangeLog:10 > + and FTL JIT layers, but he plan is to improve this support in future
"he" => "we" ?
> Source/JavaScriptCore/ChangeLog:11 > + patches.
Yeah, it is broken in DFG and FTL right now. For example, DFG prediction propagation phase for ValueAdd does not account BigInt. Before enabling BigInt, we need to complete DFG and FTL changes for BigInt. Right now, it is disabled. So it's ok to implement Baseline and LLInt first.
> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:527 > + JSBigInt* result = JSBigInt::sub(exec->vm(), WTF::get<JSBigInt*>(leftNumeric), WTF::get<JSBigInt*>(rightNumeric));
Use `vm` (defined in `BEGIN()`), instead of `exec->vm()`. And please fix `JSBigInt::unaryMinus(exec->vm()` to `JSBigInt::unaryMinus(vm` in this file.
> Source/JavaScriptCore/runtime/JSBigInt.cpp:1346 > +JSObject* JSBigInt::toObject(ExecState* state, JSGlobalObject* globalObject) const > { > - return BigIntObject::create(exec->vm(), globalObject, const_cast<JSBigInt*>(this)); > + return BigIntObject::create(state->vm(), globalObject, const_cast<JSBigInt*>(this));
Please use `exec`. In JSC, it is named `exec` basically.
> Source/JavaScriptCore/runtime/Operations.cpp:78 > + throwTypeError(callFrame, scope, ASCIILiteral("Invalid mix of BigInt and other type in addition.")); > + return { };
`return throwTypeError(callFrame, scope, ASCIILiteral("Invalid mix of BigInt and other type in addition."));` is better.
> Source/JavaScriptCore/runtime/Operations.cpp:83 > + double p1Number = WTF::get<double>(leftNumeric); > + double p2Number = WTF::get<double>(rightNumeric); > + return jsNumber(p1Number + p2Number);
Let's `return jsNumber(WTF::get<double>(leftNumeric) + WTF::get<double>(rightNumeric));`
> Source/JavaScriptCore/runtime/Operations.h:345 > + throwTypeError(state, scope, ASCIILiteral("Invalid mix of BigInt and other type in subtraction.")); > + return { };
`return throwTypeError(state, scope, ASCIILiteral("Invalid mix of BigInt and other type in subtraction."));` is better.
Caio Lima
Comment 35
2018-06-01 18:56:41 PDT
Comment on
attachment 341725
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=341725&action=review
Thank you for the review!
>> Source/JavaScriptCore/ChangeLog:11 >> + patches. > > Yeah, it is broken in DFG and FTL right now. For example, DFG prediction propagation phase for ValueAdd does not account BigInt. > Before enabling BigInt, we need to complete DFG and FTL changes for BigInt. > Right now, it is disabled. So it's ok to implement Baseline and LLInt first.
That's is the current plan, BTW.
>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:527 >> + JSBigInt* result = JSBigInt::sub(exec->vm(), WTF::get<JSBigInt*>(leftNumeric), WTF::get<JSBigInt*>(rightNumeric)); > > Use `vm` (defined in `BEGIN()`), instead of `exec->vm()`. And please fix `JSBigInt::unaryMinus(exec->vm()` to `JSBigInt::unaryMinus(vm` in this file.
Good to know. Fixed.
>> Source/JavaScriptCore/runtime/JSBigInt.cpp:1346 >> + return BigIntObject::create(state->vm(), globalObject, const_cast<JSBigInt*>(this)); > > Please use `exec`. In JSC, it is named `exec` basically.
The discussion around exec/state was started with the reason that "state" is the subject of ExecState. However, I think it is better keep consistence with all other JSC and use "exec" instead. I'm changing all usage into JSBigInt.
>> Source/JavaScriptCore/runtime/Operations.cpp:78 >> + return { }; > > `return throwTypeError(callFrame, scope, ASCIILiteral("Invalid mix of BigInt and other type in addition."));` is better.
Fixed.
>> Source/JavaScriptCore/runtime/Operations.cpp:83 >> + return jsNumber(p1Number + p2Number); > > Let's `return jsNumber(WTF::get<double>(leftNumeric) + WTF::get<double>(rightNumeric));`
Fixed
>> Source/JavaScriptCore/runtime/Operations.h:345 >> + return { }; > > `return throwTypeError(state, scope, ASCIILiteral("Invalid mix of BigInt and other type in subtraction."));` is better.
Fixed
Caio Lima
Comment 36
2018-06-01 19:22:03 PDT
Created
attachment 341815
[details]
Patch for landing
WebKit Commit Bot
Comment 37
2018-06-02 09:03:59 PDT
Comment on
attachment 341815
[details]
Patch for landing Clearing flags on attachment: 341815 Committed
r232439
: <
https://trac.webkit.org/changeset/232439
>
WebKit Commit Bot
Comment 38
2018-06-02 09:04:01 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 39
2018-06-02 09:10:06 PDT
<
rdar://problem/40748651
>
WebKit Commit Bot
Comment 40
2018-06-02 15:05:14 PDT
Re-opened since this is blocked by
bug 186238
Yusuke Suzuki
Comment 41
2018-06-02 15:05:38 PDT
Comment on
attachment 341815
[details]
Patch for landing View in context:
https://bugs.webkit.org/attachment.cgi?id=341815&action=review
> Source/JavaScriptCore/jit/JITOperations.cpp:2835 > + JSValue result = jsSub(exec, op1, op2);
I missed this, we should add exception check here: before profiling the value.
Caio Lima
Comment 42
2018-06-02 16:23:04 PDT
Comment on
attachment 341815
[details]
Patch for landing View in context:
https://bugs.webkit.org/attachment.cgi?id=341815&action=review
> Source/JavaScriptCore/runtime/JSBigInt.cpp:756 > + result->rightTrim(vm);
And I also missed this one. It returns invalid BigInt format with non-trimmed zeros.
Caio Lima
Comment 43
2018-06-02 18:41:24 PDT
Created
attachment 341860
[details]
Patch for landing
WebKit Commit Bot
Comment 44
2018-06-02 19:22:51 PDT
Comment on
attachment 341860
[details]
Patch for landing Clearing flags on attachment: 341860 Committed
r232449
: <
https://trac.webkit.org/changeset/232449
>
WebKit Commit Bot
Comment 45
2018-06-02 19:22:53 PDT
All reviewed patches have been landed. Closing bug.
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