[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

Saam Barati <sbarati at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #310597|review?, commit-queue?      |review-, commit-queue-
              Flags|                            |

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

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

> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:78
> +ENABLE_ESNEXT_OBJ_REST_SPREAD = ENABLE_ESNEXT_OBJ_REST_SPREAD;

Lets make it a runtime option, not a compile time one.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:890
> +        m_constantRegisters[entry.second].set(vm, this, JSValue(jsSet));

I don't think there is a need for the JSValue(jsSet) here.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1967
> +    unsigned index = m_nextConstantOffset;
> +    m_constantPoolRegisters.append(FirstConstantRegisterIndex + m_nextConstantOffset);
> +    ++m_nextConstantOffset;
> +    m_codeBlock->addSetConstant(set);
> +    RegisterID* m_setRegister = &m_constantPoolRegisters[index];

It looks like we do this work in a few places, perhaps we can write a function that abstracts it, like:
```
unsigned addConstantIndex()
{
    unsigned index = m_nextConstantOffset;
    m_constantPoolRegisters.append(FirstConstantRegisterIndex + m_nextConstantOffset);
    ++m_nextConstantOffset;
    return index;
}
or even make the function return std::pair<unsigned, RegisterID*>
```
and then you can just be like:
``` unsigned index = addConstantIndex();```

Maybe it's not worth it, but just a suggestion since I'm not a big fan of repeating nearly identical code.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4071
> +        excludedList = generator.emitNewArray(generator.newTemporary(), 0, 0);
> +        excludedIndex = generator.newTemporary();
> +        generator.emitLoad(excludedIndex.get(), jsNumber(0));

Please don't do this. You should just build the Set in bytecode. There is no need to first build an array, and then have your builtin convert the array to a set.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4079
> +                // Should not emit get_by_id for indexed ones.

No need for this comment. We have code like this all over the place.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:-4076
> -            RefPtr<RegisterID> propertyName = generator.emitNode(target.propertyExpression);

Can you add an assertion here of what the bindingType is? Also, I think it's worth adding an assertion that this is the last thing in the list, otherwise this algorithm is broken.

> Source/JavaScriptCore/parser/Parser.cpp:980
> +        if (kind == DestructuringKind::DestructureToExpressions)
> +        return 0;

bad indentation

> Source/JavaScriptCore/parser/Parser.cpp:1070
> +            if (UNLIKELY(match(DOTDOTDOT))) {

Is this syntactically required to be the last element in the destrucruting clause?
Also, no need for UNLIKELY, we're just going to negatively impact parsing code that uses this.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170521/565d40f4/attachment-0001.html>