Bug 148705 – [ES6] Implement ModuleNamespaceObject

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

[Yusuke Suzuki]

Implement ModuleNamespaceObject.

[Yusuke Suzuki]

Created attachment 260431 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 260431 [details]
Patch

This patch is based on the https://bugs.webkit.org/show_bug.cgi?id=148053

[Yusuke Suzuki]

Created attachment 260437 [details]
Patch

[Yusuke Suzuki]

Let's rebase it.

[Yusuke Suzuki]

Created attachment 260562 [details]
Patch

[Yusuke Suzuki]

Created attachment 260564 [details]
Patch

[Yusuke Suzuki]

Slightly clean up the comment. Now patch is ready for review.

[Geoffrey Garen]

Comment on attachment 260564 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSModuleNamespaceObject.cpp:64
> +{
> +    // http://www.ecma-international.org/ecma-262/6.0/#sec-module-namespace-exotic-objects
> +    // Quoted from the spec:
> +    //     A List containing the String values of the exported names exposed as own properties of this object.
> +    //     The list is ordered as if an Array of those String values had been sorted using Array.prototype.sort using SortCompare as comparefn.
> +    //
> +    // Sort the exported names by the code point order.
> +    Vector<UniquedStringImpl*> temporaryVector(exports.size(), nullptr);
> +    std::transform(exports.begin(), exports.end(), temporaryVector.begin(), [](const RefPtr<WTF::UniquedStringImpl>& ref) {
> +        return ref.get();
> +    });
> +    std::sort(temporaryVector.begin(), temporaryVector.end(), [] (UniquedStringImpl* lhs, UniquedStringImpl* rhs) {
> +        return codePointCompare(lhs, rhs) < 0;
> +    });
> +    for (auto* identifier : temporaryVector)
> +        m_exports.add(identifier);

This stuff should move into finishCreation. A JS object constructor should only do the bare minimum to default initialize fields in order to avoid crashing during GC. We do all meaningful work inside finishCreation in order to avoid problems that we might cause by triggering GC during a constructor. (I can see by reading this code that it doesn't cause GC, but it is preferable style to favor finishCreation so we don't have to do that reading.)

> Source/JavaScriptCore/runtime/JSModuleNamespaceObject.cpp:114
> +    // If the value is filled with TDZ value, throw the reference error.

the => a

[Yusuke Suzuki]

Comment on attachment 260564 [details]
Patch

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

Thank you for your review! I'll revise the patch based on your review and land it :D

>> Source/JavaScriptCore/runtime/JSModuleNamespaceObject.cpp:64
>> +        m_exports.add(identifier);
> 
> This stuff should move into finishCreation. A JS object constructor should only do the bare minimum to default initialize fields in order to avoid crashing during GC. We do all meaningful work inside finishCreation in order to avoid problems that we might cause by triggering GC during a constructor. (I can see by reading this code that it doesn't cause GC, but it is preferable style to favor finishCreation so we don't have to do that reading.)

Make sense. Moving the above code to the finishCreation.

>> Source/JavaScriptCore/runtime/JSModuleNamespaceObject.cpp:114
>> +    // If the value is filled with TDZ value, throw the reference error.
> 
> the => a

Thank you! Fixed.

[Yusuke Suzuki]

Committed r189429: <http://trac.webkit.org/changeset/189429>