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

Add OrderedDictionary #103309

Merged
merged 7 commits into from
Jun 14, 2024
Merged

Add OrderedDictionary #103309

merged 7 commits into from
Jun 14, 2024

Conversation

stephentoub
Copy link
Member

Closes #24826

Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

1 similar comment
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@kronic
Copy link
Contributor

kronic commented Jun 12, 2024

@stephentoub @terrajobst
Maybe it's worth adding more methods

public void Move(int oldIndex, int newIndex);
public void Stort(IComparer<TValue> comparer);

@eiriktsarpalis
Copy link
Member

Adding some kind of sorting functionality would be useful, but ideally that should be solved for IList in general, cf. #76375

This rewrites the core of the type to be based on a custom data structure rather than wrapping Dictionary and List. The core structure is based on both Dictionary in corelib and the OrderedDictionary prototype in corefxlab.

This also adds missing TryAdd, Capacity, EnsureCapacity, and TrimExcess members for parity with Dictionary, and fixes debugger views for the Key/ValueCollections.
@stephentoub
Copy link
Member Author

@eiriktsarpalis, please take another look when you get a chance. I redid the guts based on the feedback to prioritize enumeration. It's based on a combination of approaches taken by Dictionary<> and by the corefxlab OrderedDictionary<>.

@stephentoub
Copy link
Member Author

/ba-g all failures are known

@stephentoub stephentoub merged commit 2b0e517 into dotnet:main Jun 14, 2024
81 of 83 checks passed
@stephentoub stephentoub deleted the ordereddictionary branch June 14, 2024 03:08
@AndreyAkinshin
Copy link
Member

@stephentoub Thank you very much for doing this! It's fantastic that we can finally use generic OrderedDirectiory out of the box without using 3rd-party NuGet packages or implementing it from scratch.

// to record the newly updated indices.
for (i = _count - 1; i >= index; --i)
{
entries[i + 1] = entries[i];
Copy link
Member

Choose a reason for hiding this comment

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

Is there a chance we could perf improvement if the shifting of the entries array used block copying?

Copy link
Member Author

Choose a reason for hiding this comment

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

Possibly. But we still need to walk the entries individually to fix up the chains. We can more easily experiment with it once it's in a daily.

@github-actions github-actions bot locked and limited conversation to collaborators Jul 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API Proposal]: Add a generic OrderedDictionary class
6 participants