[webkit-reviews] review granted: [Bug 179002] [ESNext][BigInt] Implement support for addition operations : [Attachment 341725] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <utatane.tea at gmail.com> has granted Caio Lima
<ticaiolima at gmail.com>'s request for review:
Bug 179002: [ESNext][BigInt] Implement support for addition operations
https://bugs.webkit.org/show_bug.cgi?id=179002

Attachment 341725: Patch

https://bugs.webkit.org/attachment.cgi?id=341725&action=review




--- Comment #34 from Yusuke Suzuki <utatane.tea at gmail.com> ---
Comment on attachment 341725
  --> https://bugs.webkit.org/attachment.cgi?id=341725
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.