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.
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
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
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
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
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
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
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
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
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
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.
(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.
Created attachment 332320[details]
Benchmark Reports
This is the run comparing possible overhead of BigInt support into "+" and "-" operations. The benchmarks are neutral.
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.
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
2018-01-21 15:56 PST, Caio Lima
2018-01-21 16:51 PST, EWS Watchlist
2018-01-21 16:59 PST, EWS Watchlist
2018-01-21 17:09 PST, EWS Watchlist
2018-01-21 17:16 PST, EWS Watchlist
2018-01-23 14:37 PST, Caio Lima
2018-01-23 15:26 PST, Caio Lima
2018-01-23 16:40 PST, EWS Watchlist
2018-01-23 16:41 PST, EWS Watchlist
2018-01-23 17:08 PST, EWS Watchlist
2018-01-23 17:26 PST, EWS Watchlist
2018-01-24 02:48 PST, Caio Lima
2018-01-24 04:03 PST, EWS Watchlist
2018-01-24 14:04 PST, Caio Lima
2018-01-25 14:31 PST, Caio Lima
2018-01-25 14:44 PST, Caio Lima
2018-01-25 14:46 PST, Caio Lima
2018-05-31 20:00 PDT, Caio Lima
2018-06-01 07:00 PDT, Caio Lima
2018-06-01 19:22 PDT, Caio Lima
2018-06-02 18:41 PDT, Caio Lima