Bug 144111 - [ES6] Destructuring assignment need to accept iterables
Summary: [ES6] Destructuring assignment need to accept iterables
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-23 12:08 PDT by Yusuke Suzuki
Modified: 2015-06-20 04:05 PDT (History)
8 users (show)

See Also:


Attachments
Prototype (13.22 KB, patch)
2015-06-17 14:51 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (24.09 KB, patch)
2015-06-19 12:33 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (22.81 KB, patch)
2015-06-19 12:38 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (22.42 KB, patch)
2015-06-19 12:43 PDT, Yusuke Suzuki
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2015-04-23 12:08:35 PDT
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.
Comment 1 Yusuke Suzuki 2015-06-17 14:51:50 PDT
Created attachment 255038 [details]
Prototype

Initial prototype
Comment 2 Yusuke Suzuki 2015-06-19 12:33:09 PDT
Created attachment 255213 [details]
Patch
Comment 3 Yusuke Suzuki 2015-06-19 12:38:28 PDT
Created attachment 255214 [details]
Patch
Comment 4 Yusuke Suzuki 2015-06-19 12:43:46 PDT
Created attachment 255216 [details]
Patch
Comment 5 Yusuke Suzuki 2015-06-19 14:39:17 PDT
OK, now the patch is ready.
Could anyone take a look?
Comment 6 Darin Adler 2015-06-19 16:21:19 PDT
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.
Comment 7 Yusuke Suzuki 2015-06-20 04:03:40 PDT
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.
Comment 8 Yusuke Suzuki 2015-06-20 04:05:16 PDT
Committed r185791: <http://trac.webkit.org/changeset/185791>