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

Implementing stable sort #809

Merged
merged 5 commits into from Dec 10, 2020
Merged

Implementing stable sort #809

merged 5 commits into from Dec 10, 2020

Conversation

sebastienros
Copy link
Owner

Fixes #808

@lahma
Copy link
Collaborator

lahma commented Nov 30, 2020

Seems to hang?

@@ -68,6 +70,60 @@ public virtual JsValue[] GetAll(Types elementTypes)

public abstract void DeletePropertyOrThrow(ulong index);

public IEnumerator<JsValue> GetEnumerator()
Copy link
Collaborator

Choose a reason for hiding this comment

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

If there's an overload of concrete type public ArrayOperationsIterator GetEnumerator() then there would be no dispatch overhead like the interfaces have, CLR will find the version by name.

Copy link
Owner Author

Choose a reason for hiding this comment

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

What do you suggest? To implement it in each concrete type and call base.GetEnumerator() ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was referring to concrete enumerator method without interface, as seen here:

/// <summary>
/// Gets an enumerator over the dictionary
/// </summary>
public Enumerator GetEnumerator() => new Enumerator(this); // avoid boxing
/// <summary>
/// Gets an enumerator over the dictionary
/// </summary>
IEnumerator<KeyValuePair<TKey, TValue>> IEnumerable<KeyValuePair<TKey, TValue>>.GetEnumerator() =>
new Enumerator(this);
/// <summary>
/// Gets an enumerator over the dictionary
/// </summary>
IEnumerator IEnumerable.GetEnumerator() => new Enumerator(this);

Explicit interface implementations for generic use case and then one of concrete type to avoid boxing (struct via interface).

Copy link
Owner Author

Choose a reason for hiding this comment

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

Updated but this new member is not called, and we need to user OrderBy for a stable sort.

@sebastienros
Copy link
Owner Author

There is a test that must be blocked in an infinite loop/ Haven't identified which one yet.

@sebastienros
Copy link
Owner Author

@lahma new review please

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.

Looks good and contained. I think that at some point it would make sense to make the actual ObjectInstance and ArrayInstance implement the IEnumerable<JsValue>, this would open resolution for extension methods too.

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.

Array.prototype.sort is unstable
2 participants