[webkit-reviews] review denied: [Bug 167962] [ESnext] Implement Object Rest - Implementing Object Rest Destructuring : [Attachment 310597] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied Caio Lima <ticaiolima at gmail.com>'s
request for review:
Bug 167962: [ESnext] Implement Object Rest - Implementing Object Rest
Destructuring
https://bugs.webkit.org/show_bug.cgi?id=167962

Attachment 310597: Patch

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




--- 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.