[Webkit-unassigned] [Bug 167962] [ESnext] Implement Object Rest - Implementing Object Rest Destructuring

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=167962

Keith Miller <keith_miller at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #301324|review?                     |review-
              Flags|                            |

--- Comment #15 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 301324
  --> https://bugs.webkit.org/attachment.cgi?id=301324
Patch

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

I think it looks pretty good. I have a few comments, however.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4051
> +            if (UNLIKELY(m_containsRestElement)) {
> +                RefPtr<RegisterID> propertyName = generator.emitLoad(nullptr, target.propertyName);
> +                generator.emitDirectPutByVal(excludedList.get(), excludedIndex.get(), propertyName.get());
> +                generator.emitInc(excludedIndex.get());

Don't we statically know what we are going to put in the excludedList at this point? Can we just add this array to the constant pool? If we do, we should probably freeze so the array can't be modified.

> Source/JavaScriptCore/parser/Parser.cpp:1019
> +                if (kind == DestructuringKind::DestructureToExpressions && !innerPattern)
> +                    return 0;
> +                failIfFalse(innerPattern, "Cannot parse this destructuring pattern");

I think you just want to do propageError(); here

> Source/JavaScriptCore/runtime/JSGlobalObject.h:117
> +    macro(Set, set, set, JSSet, Set, object) \

Why did you move this?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170222/a6e5b769/attachment.html>