[webkit-reviews] review granted: [Bug 157972] super should not depend on __proto__ : [Attachment 400794] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Alexey Shvayka
<shvaikalesh at gmail.com>'s request for review:
Bug 157972: super should not depend on __proto__
https://bugs.webkit.org/show_bug.cgi?id=157972

Attachment 400794: Patch

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




--- Comment #13 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 400794
  --> https://bugs.webkit.org/attachment.cgi?id=400794
Patch

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

r=me with a few comments

> Source/JavaScriptCore/jit/JITOpcodes.cpp:1528
> +void JIT::emit_op_get_prototype_of(const Instruction* currentInstruction)

Can we also add fast paths for DFG/FTL that look like this? Maybe in a
follow-up?

> Source/JavaScriptCore/jit/JITOpcodes.cpp:1537
> +    addSlowCase(branchTest32(NonZero, Address(regT2,
Structure::outOfLineTypeFlagsOffset()), TrustedImm32(OverridesGetPrototype >>
8)));

nit: can we define 8 somewhere in JSTypeInfo like "numberOfInlineBits"?

>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1492
>> +	const OverridesGetPrototype = 0x400 # (1 << 18) >> 8
> 
> We can improve this by tweaking LLInt parser to support bitwise shifts in a
follow-up.

constexpr expressions in LLInt doesn't work here?

If not, I'd prefer to define this in the JSTypeInfo header and just use the
value here, and we can static assert it's proper there? Could also be used in
baseline JIT code for this.

> Source/JavaScriptCore/runtime/JSTypeInfo.h:60
> +static constexpr unsigned OverridesGetPrototype = 1 << 18;

Can you add validation for this into the new Structure::validateFlags that was
added after you uploaded this change.