Skip to content

Implement Enumerable.Reverse(TSource[])#107957

Merged
333fred merged 4 commits intomainfrom
work/333fred/enumerable.reverse
Sep 25, 2024
Merged

Implement Enumerable.Reverse(TSource[])#107957
333fred merged 4 commits intomainfrom
work/333fred/enumerable.reverse

Conversation

@333fred
Copy link
Copy Markdown
Member

@333fred 333fred commented Sep 17, 2024

Closes #107723.

@ghost
Copy link
Copy Markdown

ghost commented Sep 17, 2024

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
Copy link
Copy Markdown

ghost commented Sep 17, 2024

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.

@333fred
Copy link
Copy Markdown
Member Author

333fred commented Sep 17, 2024

Well, I tried opening in draft to avoid tagging people until I was sure tests were passing...

Comment thread src/libraries/System.Linq/src/System/Linq/Reverse.cs Outdated
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.source);
}

if (source.Length == 0)
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.

Nit: I would probably just have this method call the existing Reverse directly, although I see why you might have wanted to avoid a redundant type check.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, my thought process was "I could just call Reverse, but it's not like this a complicated method or changing very often, and this is more efficient".

T[] expected = source.ToArray();
Array.Reverse(expected);

IEnumerable<T> actual = source.ToArray().Reverse();
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.

Does this test exercise the case where source is empty?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, an empty array is in the input data.

Copy link
Copy Markdown
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.

Thanks

@333fred 333fred marked this pull request as ready for review September 19, 2024 18:42
nameof(Enumerable.Prepend),
nameof(Enumerable.ToHashSet),
nameof(Enumerable.TryGetNonEnumeratedCount),
nameof(Enumerable.Reverse),
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 wish there was a simple way to distinguish between the different overloads, but I suppose this should suffice.

@333fred 333fred merged commit d6ac24c into main Sep 25, 2024
@333fred 333fred deleted the work/333fred/enumerable.reverse branch September 25, 2024 17:18
sirntar pushed a commit to sirntar/runtime that referenced this pull request Sep 30, 2024
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 26, 2024
@MihaZupan MihaZupan added this to the 10.0.0 milestone Jan 17, 2025
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]: Enumerable.Reverse<T>(T[])

3 participants