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
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
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
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
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
Patch (73.00 KB, patch)
2018-01-23 14:37 PST, Caio Lima
no flags
Patch (73.00 KB, patch)
2018-01-23 15:26 PST, Caio Lima
no flags
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
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
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
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
Patch (70.55 KB, patch)
2018-01-24 02:48 PST, Caio Lima
no flags
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
Patch (70.54 KB, patch)
2018-01-24 14:04 PST, Caio Lima
no flags
Patch (73.25 KB, patch)
2018-01-25 14:31 PST, Caio Lima
no flags
Patch (73.23 KB, patch)
2018-01-25 14:44 PST, Caio Lima
no flags
Benchmark Reports (87.65 KB, text/plain)
2018-01-25 14:46 PST, Caio Lima
no flags
Patch (60.80 KB, patch)
2018-05-31 20:00 PDT, Caio Lima
ysuzuki: review+
Benchmark Report (93.41 KB, text/plain)
2018-06-01 07:00 PDT, Caio Lima
no flags
Patch for landing (69.52 KB, patch)
2018-06-01 19:22 PDT, Caio Lima
no flags
Patch for landing (68.05 KB, patch)
2018-06-02 18:41 PDT, Caio Lima
no flags
Caio Lima
Comment 1 2018-01-21 15:56:23 PST
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
Caio Lima
Comment 12 2018-01-23 15:26:23 PST
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
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
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
Caio Lima
Comment 29 2018-01-25 14:44:26 PST
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
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
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.