[webkit-reviews] review granted: [Bug 186229] [ESNext][BigInt] Implement support for "|" : [Attachment 351248] WIP - RFC Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <yusukesuzuki at slowstart.org> has granted	review:
Bug 186229: [ESNext][BigInt] Implement support for "|"
https://bugs.webkit.org/show_bug.cgi?id=186229

Attachment 351248: WIP - RFC Patch

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




--- Comment #12 from Yusuke Suzuki <yusukesuzuki at slowstart.org> ---
Comment on attachment 351248
  --> https://bugs.webkit.org/attachment.cgi?id=351248
WIP - RFC Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351248&action=review

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:129
> +		       CRASH();

DFG_CRASH is preferable.

> Source/JavaScriptCore/dfg/DFGOperations.cpp:341
> +inline EncodedJSValue bitwiseOp(ExecState* exec, EncodedJSValue encodedOp1,
EncodedJSValue encodedOp2, std::function<JSBigInt*(JSBigInt*, JSBigInt*)>
bigIntOp, std::function<int32_t(int32_t, int32_t)> int32Op)

Use `template<typename BigIntOperation>` and `BigIntOperation&& bigIntOp`
instead of `std::function<>`.
std::function could cause unnecessary heap allocation.
And even if std::function<> is required (to store it somewhere), use
WTF::Function<> instead. => do not use std::function<> at any time :P

> Source/JavaScriptCore/dfg/DFGOperations.cpp:371
> +    auto bigIntOp = [vm] (JSBigInt* left, JSBigInt* right) -> JSBigInt* {

Instead of capturing VM here, taking `VM& vm` parameter and passing VM in the
caller function (bitwiseOp) is cleaner.

> Source/JavaScriptCore/dfg/DFGOperations.cpp:385
> +    auto bigIntOp = [vm] (JSBigInt* left, JSBigInt* right) -> JSBigInt* {

Ditto.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:390
> +	   return absoluteAddOne(vm, result, SignOption::Signed);

Nice.