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

No-op ConcurrentStack.PushRange(T[]) if empty #62126

Merged
merged 4 commits into from
Nov 30, 2021

Conversation

martincostello
Copy link
Member

Resolves #62121.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 29, 2021
@ghost
Copy link

ghost commented Nov 29, 2021

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

Issue Details

Resolves #62121.

Author: martincostello
Assignees: -
Labels:

area-System.Collections, community-contribution

Milestone: -

Assert that ArgumentOutOfRangeException is thrown if specifying indexes for an empty array with PushRange(T[], int, int).
@stephentoub
Copy link
Member

stephentoub commented Nov 29, 2021

Thanks.

This would address the case of PushRange(empty), but not of PushRange(empty, 0, 0), which should be functionally identical.

I suggest the right fix is simply to change this condition:


from

if (startIndex >= length || startIndex < 0)

to

if (startIndex > length || startIndex < 0)

or even:

if ((uint)startIndex > length)

i.e. it should be fine to have the start index be at the end of the array, as long as the count is 0, which the subsequent check validates.

@martincostello
Copy link
Member Author

Thanks both - I did think about that, but then asking to start from index 0 (which doesn't exist) seemed to be an equally valid mistake to me at first sight on an empty array when the range is specified, so went with this initially to gauge thoughts.

I'll update the PR as suggested 👍

Allow out-of-bounds index on empty array if count is zero.
@stephentoub
Copy link
Member

asking to start from index 0 (which doesn't exist) seemed to be an equally valid mistake to me

This is how things generally work in .NET, e.g. you can insert at the end of a collection with the index == length, you can take a 0-length substring at the end of a string, etc.

Add a further test case for a non-empty array but with a count parameter value of zero.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Collections community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ConcurrentStack.PushRange(T[]) throws an ArgumentOutOfRangeException on empty T[]
4 participants