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 span-based static Create overloads for immutable collections #87945

Merged
merged 1 commit into from Jun 23, 2023

Conversation

stephentoub
Copy link
Member

Contributes to #87569

@dotnet-issue-labeler
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.

@ghost ghost assigned stephentoub Jun 22, 2023
@ghost
Copy link

ghost commented Jun 22, 2023

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

Issue Details

Contributes to #87569

Author: stephentoub
Assignees: -
Labels:

area-System.Collections

Milestone: 8.0.0

/// <typeparam name="T">The type of items stored by the collection.</typeparam>
/// <param name="items">The items to prepopulate.</param>
/// <returns>The new immutable collection.</returns>
public static ImmutableHashSet<T> Create<T>(ReadOnlySpan<T> items)
{
return ImmutableHashSet<T>.Empty.Union(items);
Copy link
Member

Choose a reason for hiding this comment

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

I know this predates your changes, but it strikes me as odd that we're defining a core factory method in terms of a "Union" operation. Not familiar with the Union implementation but it sounds like it might be introducing unnecessary checks on the LHS?

Copy link
Member Author

Choose a reason for hiding this comment

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

Potentially. Happy for us to address any overheads here separately.

Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

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

LGTM but given that the main collection literals API proposal isn't finalized yet is there a possibility that a different API shape could be necessary?

@stephentoub
Copy link
Member Author

LGTM but given that the main collection literals API proposal isn't finalized yet is there a possibility that a different API shape could be necessary?

It's possible, but we want these anyway for devs to use directly :) We'd like them on the other collections like List and HashSet as well, but we can't add the overloads there we want them yet because they result in ambiguity errors for some callers; that problem doesn't exist for the immutable collections given their current shape.

@stephentoub stephentoub merged commit de857cf into dotnet:main Jun 23, 2023
105 checks passed
@stephentoub stephentoub deleted the immutablecollectionscreate branch June 23, 2023 14:43
@dotnet dotnet locked as resolved and limited conversation to collaborators Jul 23, 2023
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.

None yet

2 participants