[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 #302774|review?                     |review-
              Flags|                            |

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

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

I think it's looking pretty good, I have a couple of style comments, however.

> Source/JavaScriptCore/builtins/GlobalOperations.js:95
> +        if (!excludedSet.has(nextKey)) {

I think you need to make private symbols for these properties. If a user deletes the has property from Set.prototype your code won't work.

> Source/JavaScriptCore/builtins/GlobalOperations.js:99
> +                @Object.defineProperty(target, nextKey, {value: propValue, enumerable: true, writable: true, configurable: true});

ditto.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1944
> +    if (!setConstantDeconstructionSetRegisters(vm, unlinkedCodeBlock->constantDestructSets()))

I would call this setConstantIdentifierSetRegisters.

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:71
> +typedef std::pair<HashSet<UniquedStringImpl*>, unsigned> ConstantDeconstructSetEntry;

I would call this ConstantIndentifierSetEntry

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:230
> +    const Vector<ConstantDeconstructSetEntry>& constantDestructSets() { return m_constantObjDeconstructSets; }

Style: We normally don't abbreviate names like this.

but I would also change this to constantIdentifierSets().

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:461
> +    Vector<ConstantDeconstructSetEntry> m_constantObjDeconstructSets;

ditto.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1927
> +        if (!(entry.first == set))

I would do if (entry.first != set), although you might need to add an operator != to HashSet.

> Source/JavaScriptCore/runtime/ScriptExecutable.cpp:193
> +            // At this point, the CodeBlock creation can fail trying to
> +            // allocate JSSet constants in CodeBlock::setConstantDeconstructionSetRegister.
> +            // Since the only exception from it is OOM, we throw this kind of exception.

I don't think you need this comment.

-- 
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/20170303/d684e5ac/attachment.html>