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

Use Array.Empty<T> for LINQ's Enumerable.Empty<TResult>#2290

Merged
stephentoub merged 1 commit into
dotnet:masterfrom
justinvp:linqempty
Jul 9, 2015
Merged

Use Array.Empty<T> for LINQ's Enumerable.Empty<TResult>#2290
stephentoub merged 1 commit into
dotnet:masterfrom
justinvp:linqempty

Conversation

@justinvp

@justinvp justinvp commented Jul 9, 2015

Copy link
Copy Markdown
Contributor

No description provided.

stephentoub
stephentoub reviewed Jul 9, 2015
View reviewed changes
Comment thread src/System.Linq/src/System/Linq/Enumerable.cs Outdated

@stephentoub stephentoub Jul 9, 2015

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.

I don't believe this comment is accurate any more. Enumerating an empty array should not allocate a new enumerator: https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/Array.cs#L2717

@justinvp justinvp Jul 9, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Excellent! I'll remove the comment.

@justinvp justinvp Jul 9, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As an aside, correct me if I am wrong, but it looks like SZGenericArrayEnumerator<T>.Empty is being greedily allocated as soon as SZGenericArrayEnumerator<T> is used for the first time (first time the generic GetEnumerator is called). Which means lots of possible unnecessary empty enumerator allocations for each T. Wouldn't it be better if this was lazily allocated only when an empty enumerator was actually needed? Something like EmptySZGenericArrayEnumerator<T>.Instance. (I can open an issue/PR on CoreCLR to continue the discussion there, if warranted).

@stephentoub stephentoub Jul 9, 2015

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.

Yeah, that's probably a good idea.

@justinvp justinvp Jul 9, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Never mind -- I tested it out and the runtime is doing the right thing.

@stephentoub

stephentoub commented Jul 9, 2015

Copy link
Copy Markdown
Member

cc: @VSadov

@VSadov

VSadov commented Jul 9, 2015

Copy link
Copy Markdown
Member

LGTM

stephentoub added a commit that referenced this pull request Jul 9, 2015
@stephentoub
Merge pull request #2290 from justinvp/linqempty
b219b8d 
Use Array.Empty<T> for LINQ's Enumerable.Empty<TResult>
@stephentoub stephentoub merged commit b219b8d into dotnet:master Jul 9, 2015
@justinvp justinvp deleted the linqempty branch October 1, 2015 22:33
@karelz karelz modified the milestone: 1.0.0-rtm Dec 3, 2016
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
@stephentoub
Merge pull request dotnet/corefx#2290 from justinvp/linqempty
de49963 
Use Array.Empty<T> for LINQ's Enumerable.Empty<TResult>

Commit migrated from dotnet/corefx@b219b8d
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.0.0-rtm

Development

Successfully merging this pull request may close these issues.

5 participants

@justinvp @stephentoub @VSadov @karelz @dnfclas