[webkit-reviews] review granted: [Bug 208261] Poly proto should work with property delete transitions : [Attachment 391905] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Justin Michaud
<justin_michaud at apple.com>'s request for review:
Bug 208261: Poly proto should work with property delete transitions
https://bugs.webkit.org/show_bug.cgi?id=208261

Attachment 391905: Patch

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




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

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

r=me

> Source/JavaScriptCore/ChangeLog:9
> +	   and poly photo cause us to cache a setter call along a prototype
chain that 

photo => proto

> Source/JavaScriptCore/ChangeLog:10
> +	   is no longer the correct setter to call. This is exposed as a result
of 206430

nit: I'd link to a revision instead of "206430", since "206430" is slightly
ambiguous without a link

> Source/JavaScriptCore/ChangeLog:24
> +	   were before it was called. We see that A's setter still exists, so
we cache it
> +	   without ever checking that it is still a setter.

This sentence doesn't make sense.

I think you also need to say what your fix actually is. All you're doing is
describing a problem.

Can you also link to bugs for fixing the semantic issues that remain?

> JSTests/stress/delete-property-poly-proto.js:1
> +//@ requireOptions("--forcePolyProto=1", "--useLLInt=0")

nit: use 4 space indent

> JSTests/stress/poly-proto-setter-adds-setter-in-middle.js:32
> +//if (!correct)
> +//	 throw new Error()

I think you shouldn't have commented out code here, instead, you should do:

FIXME: Assert that correct is true <bug link here>