Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Add HashSet<T> ctors with capacity#2862

Merged
stephentoub merged 1 commit intodotnet:futurefrom
stephentoub:hashset_ctors
Aug 18, 2015
Merged

Add HashSet<T> ctors with capacity#2862
stephentoub merged 1 commit intodotnet:futurefrom
stephentoub:hashset_ctors

Conversation

@stephentoub
Copy link
Copy Markdown
Member

Replaces PR #2122. I squashed and fixed up @MarkusSintonen's commits from that PR.
Fixes #382.

New tests are not included due to System.Collections.dll being a partial facade, and the repo not currently having the infrastructure to enable adding tests for new surface area in partial facades.

cc: @terrajobst, @KrzysztofCwalina

Adds ctors to HashSet that accept a capacity.

New tests are not included due to System.Collections.dll being
a partial facade, and the repo not currently having the infrastructure
to enable adding tests for new surface area in partial facades.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder (not tied to this commit) if we should strip Contract.EndContractBlock et al since we're not using the code contract tooling.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It is something definitely worth considering, to this point I've not seen the value of ripping it all out yet. It may still be somewhat useful if we do get the C# vNext contracts feature in providing some hints about the contracts.

@nguerrera
Copy link
Copy Markdown
Contributor

LGTM

It might be worth merging master -> future and updating the contracts under the new ref/ now that they're part of the repo. It would also be nice to make sure there are (internal) tracking issues to get new surface area backported to desktop. It's not necessary to block on that for this commit, but perhaps we should open an issue to do those things for any changes already in future. cc @weshaggard

@stephentoub
Copy link
Copy Markdown
Member Author

CI failed due to future needing to be updated with some recent commits to master. I'll take care of that tomorrow if no one beats me to it.

@weshaggard
Copy link
Copy Markdown
Member

@nguerrera @stephentoub it is my expectation that at some point when we decide to merge future back to master someone will need to pay the debt of ensuring the new APIs are added to the correct contracts and supported on the appropriate platforms. It is probably worth creating a github issue tracking this merge and listing pointers to the commits/issues that we have accepted in future for easier tracking. I also agree with @nguerrera that we can start adding these APIs into the ref for the given contract which will also aid in identifying public API changes.

@stephentoub
Copy link
Copy Markdown
Member Author

It is probably worth creating a github issue tracking this merge and listing pointers to the commits/issues that we have accepted in future for easier tracking

Great. Please do 😉

@weshaggard
Copy link
Copy Markdown
Member

@stephentoub I've created item https://github.com/dotnet/corefx/issues/2869 to track the merge of future to master. I will try and do a pass over the existing PR's and add links into the main item there, but please feel free to assist with the links and keeping it updated.

@stephentoub
Copy link
Copy Markdown
Member Author

@dotnet-bot test this please

stephentoub added a commit that referenced this pull request Aug 18, 2015
Add HashSet<T> ctors with capacity
@stephentoub stephentoub merged commit 2b4ee63 into dotnet:future Aug 18, 2015
@stephentoub stephentoub deleted the hashset_ctors branch August 18, 2015 18:13
stephentoub added a commit to stephentoub/corefx that referenced this pull request Aug 18, 2015
@karelz karelz modified the milestone: Future Jan 25, 2017
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
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.

7 participants