Bug 144111 – [ES6] Destructuring assignment need to accept iterables

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

Current destructuring assignment is implemented for object and array.
In array case, we need to take a generic way. Iterating the provided value through iterator interface.

[Yusuke Suzuki]

Created attachment 255038 [details]
Prototype

Initial prototype

[Yusuke Suzuki]

Created attachment 255213 [details]
Patch

[Yusuke Suzuki]

Created attachment 255214 [details]
Patch

[Yusuke Suzuki]

Created attachment 255216 [details]
Patch

[Yusuke Suzuki]

OK, now the patch is ready.
Could anyone take a look?

[Darin Adler]

Comment on attachment 255216 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:12
> +        The iteration becomes different from the for-of cahse.

stray "h" here

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3134
> +    RefPtr<RegisterID> iterator = nullptr;

No need to explicitly state "= nullptr". I think it’s fine to leave it out.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3136
> +        iterator = generator.emitGetById(generator.newTemporary(), rhs, generator.propertyNames().iteratorSymbol);

But why not initialize iterator outside the scope instead of here? No need to set it to null first.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3142
> +    RefPtr<RegisterID> done = generator.newTemporary();

I have an idea for a slightly different structure that can use a modern for loop. Leave done initialized to nullptr here.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3145
>      for (size_t i = 0; i < m_targetPatterns.size(); i++) {

Use a modern for loop here:

    for (auto& target : m_targetPatterns) {

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3150
> +        if (i)
> +            generator.emitJumpIfTrue(done.get(), iterationSkipped.get());

Then, here, write:

    if (!done)
        done = generator.newTemporary();
    else
        generator.emitJumpIfTrue(done.get(), iterationSkipped.get());

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3176
> +        auto& target = m_targetPatterns[i];

And you don’t need this line of code. Seems slightly cleaner to me.

> Source/JavaScriptCore/parser/Parser.cpp:626
> +        JSTextPosition divotEnd = lastTokenEndPosition();

Not sure we need a local variable for divotEnd.

[Yusuke Suzuki]

Comment on attachment 255216 [details]
Patch

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

Thank you for your review!

>> Source/JavaScriptCore/ChangeLog:12
>> +        The iteration becomes different from the for-of cahse.
> 
> stray "h" here

Fixed.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3136
>> +        iterator = generator.emitGetById(generator.newTemporary(), rhs, generator.propertyNames().iteratorSymbol);
> 
> But why not initialize iterator outside the scope instead of here? No need to set it to null first.

OK, I'll initialize iterator as `generator.newTemporary()`.
This block early releases CallArguments.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3176
>> +        auto& target = m_targetPatterns[i];
> 
> And you don’t need this line of code. Seems slightly cleaner to me.

Oh, that looks very nice! I've changed it. Thank you!

>> Source/JavaScriptCore/parser/Parser.cpp:626
>> +        JSTextPosition divotEnd = lastTokenEndPosition();
> 
> Not sure we need a local variable for divotEnd.

OK, just using `lastTokenEndPosition()` directly.

[Yusuke Suzuki]

Committed r185791: <http://trac.webkit.org/changeset/185791>