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

Misc improvements of OneOrMany #68948

Merged
merged 3 commits into from
Jul 14, 2023
Merged

Misc improvements of OneOrMany #68948

merged 3 commits into from
Jul 14, 2023

Conversation

tmat
Copy link
Member

@tmat tmat commented Jul 10, 2023

Adds custom debugger display.
Adds SequenceEqual and a few convenience helpers.

@tmat tmat requested review from a team as code owners July 10, 2023 00:36
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 10, 2023
@jaredpar jaredpar removed the untriaged Issues and PRs which have not yet been triaged by a lead label Jul 10, 2023
@CyrusNajmabadi
Copy link
Member

this type seems to have a lot of brokennes :'(

@jaredpar jaredpar added this to the 17.8 milestone Jul 12, 2023
if (many is [var item])
{
_one = item;
_many = default;
Copy link
Member

Choose a reason for hiding this comment

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

I understand this makes the internal invariants of the type a lot simpler but it does mean we essentially introduce allocations for this pattern where before there was no allocation:

var array = ImmutableArray.Create<int>(42);
var oneOrMany = new OneOrMany<int>(array);
// Before no alloc, after allocs 
oneOrMany.ToImmutable();

Are we concerned this could regress allocations? Don't have a guid intuition on how common this would be.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, potentially. I don't think it's common since we usually know at creation time that we only have a single element so we wouldn't create that array.

@tmat tmat merged commit a2faca7 into dotnet:main Jul 14, 2023
@tmat tmat deleted the OneOrMany branch July 14, 2023 20:56
@ghost ghost modified the milestones: 17.8, Next Jul 14, 2023
@allisonchou allisonchou removed this from the Next milestone Jul 24, 2023
@allisonchou allisonchou added this to the 17.8 P1 milestone Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants