[webkit-reviews] review granted: [Bug 213372] [JSC][ESNext] Create a new opcode to handle private fields store/define : [Attachment 409344] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Caio Lima
<ticaiolima at gmail.com>'s request for review:
Bug 213372: [JSC][ESNext] Create a new opcode to handle private fields
store/define
https://bugs.webkit.org/show_bug.cgi?id=213372

Attachment 409344: Patch

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




--- Comment #47 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 409344
  --> https://bugs.webkit.org/attachment.cgi?id=409344
Patch

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

r=me

> Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:170
> +    default:
> +	   RELEASE_ASSERT_NOT_REACHED();
> +	   return nullptr;

Let's remove this default clause. To make it possible, let's make PutKind enum
class. Then, we can purge this default clause. This make it possible that
future extension of PutKind will warn at compile time.

> Source/JavaScriptCore/jit/Repatch.cpp:555
> +    default:
> +	   RELEASE_ASSERT_NOT_REACHED();
> +	   return nullptr;

Ditto.

> Source/JavaScriptCore/jit/Repatch.cpp:578
> +    default:
> +	   RELEASE_ASSERT_NOT_REACHED(); 
> +	   return nullptr;

Ditto.

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1189
>      if (subscript.isDouble()) {
> -	   ASSERT(!isPrivateFieldAccess);
>	   double subscriptAsDouble = subscript.asDouble();
>	   uint32_t subscriptAsUInt32 =
static_cast<uint32_t>(subscriptAsDouble);
>	   if (subscriptAsDouble == subscriptAsUInt32 &&
isIndex(subscriptAsUInt32)) {

OK, now I understand. `subscript.tryGetAsUint32Index` subsumes this branch. So,
this is dead code. Then we can remove it.

> Source/JavaScriptCore/runtime/PrivateFieldPutKind.cpp:39
> +    if (isSet())
> +	   out.print("Set");
> +    else
> +	   out.print("Define");

Let's support None too.