Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement WeakMap and WeakSet #912

Merged
merged 2 commits into from May 31, 2021
Merged

Conversation

jfd16
Copy link
Contributor

@jfd16 jfd16 commented May 31, 2021

No description provided.

@lahma
Copy link
Collaborator

lahma commented May 31, 2021

Thanks for creating this! Can you please add official ECMA tests too under Jint.Tests.Test262\BuiltIns?

using Xunit;

namespace Jint.Tests.Test262.BuiltIns
{
    public class WeakMapTests : Test262Test
    {
        [Theory(DisplayName = "built-ins\\WeakMap")]
        [MemberData(nameof(SourceFiles), "built-ins\\WeakMap", false)]
        [MemberData(nameof(SourceFiles), "built-ins\\WeakMap", true, Skip = "Skipped")]
        protected void Symbol(SourceFile sourceFile)
        {
            RunTestInternal(sourceFile);
        }
    }

    public class WeakSetTests : Test262Test
    {
        [Theory(DisplayName = "built-ins\\WeakSet")]
        [MemberData(nameof(SourceFiles), "built-ins\\WeakSet", false)]
        [MemberData(nameof(SourceFiles), "built-ins\\WeakSet", true, Skip = "Skipped")]
        protected void Symbol(SourceFile sourceFile)
        {
            RunTestInternal(sourceFile);
        }
    }
}

Copy link
Collaborator

@lahma lahma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking top-notch, couple remarks and if you want to, you could also tick the feature to be present in the main README.md

_table = new ConditionalWeakTable<JsValue, JsValue>();
}

public override PropertyDescriptor GetOwnProperty(JsValue property)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed?

namespace Jint.Native.WeakMap
{
/// <summary>
/// https://262.ecma-international.org/11.0/#sec-weakmap-objects
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the living spec links can be more useful as they are not tied to specific version https://tc39.es/ecma262/#sec-weakmap-objects

_table = new ConditionalWeakTable<JsValue, object>();
}

public override PropertyDescriptor GetOwnProperty(JsValue property)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this needed?

namespace Jint.Native.WeakSet
{
/// <summary>
/// https://262.ecma-international.org/11.0/#sec-weakset-objects
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: living spec would be better

@lahma lahma merged commit 5a058bf into sebastienros:main May 31, 2021
@lahma
Copy link
Collaborator

lahma commented May 31, 2021

Thank you once again, great addition 🚀

@jfd16 jfd16 deleted the weakmap-weakset branch May 31, 2021 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants