[webkit-reviews] review granted: [Bug 186228] [ESNext][BigInt] Implement support for "&" : [Attachment 351082] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <yusukesuzuki at slowstart.org> has granted Caio Lima
<ticaiolima at gmail.com>'s request for review:
Bug 186228: [ESNext][BigInt] Implement support for "&"
https://bugs.webkit.org/show_bug.cgi?id=186228

Attachment 351082: Patch

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




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

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

r=me with fixes.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4848
> +	       if (isInt32Speculation(getPredictionWithoutOSRExit()))

Use `getPrediction()`. We always profile the value appropriately.

> Source/JavaScriptCore/jit/JIT.h:797
> +	   void emitBitBinaryOpFastPath(Instruction* currentInstruction, bool
shouldEmitProfiling = false);

Let's have `enum class` for profiling mode instead of `bool`.

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1162
> +    valueProfile(t0, t3, 16, t2)

valueProfile's argument seems wrong. `valueProfile(tag, payload, operand,
scratch)`.
And can we make `16` `((advance - 1) * 4)`?

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1106
> +    valueProfile(t0, 4, t2)

Can we use `(advance - 1)` instead of `4`?

> Source/JavaScriptCore/runtime/JSBigInt.cpp:112
>      } else {
>	   bigInt->setDigit(0, static_cast<Digit>(value));
> -	   bigInt->setSign(false);
>      }

Remove `{` and `}`.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:140
>	   } else {
>	       bigInt->setDigit(0, static_cast<Digit>(value));
> -	       bigInt->setSign(false);
>	   }

Remove `{` and `}`.

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:758
> +    if (isDouble() && canBeInt32(asDouble()))
> +	   return JSC::toInt32(asDouble());

I think `return static_cast<int32_t>(asDouble());` is OK since canBeInt32 goes
well.
Other cases go to the generic path below.