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

Updated Exception Handling for Collection<T> #23290

Merged
merged 5 commits into from
Mar 22, 2019
Merged

Updated Exception Handling for Collection<T> #23290

merged 5 commits into from
Mar 22, 2019

Conversation

SkyeHoefling
Copy link

Part of #23166

This change addresses follow-up concerns on how we are throwing different exceptions in the Collection class for add range APIs which are used ObservableCollection as well.

@stephentoub highlighted several follow up concerns which I believe I have covered in this PR and in the comment here.

#23166 (comment)
#23166 (comment)

There's no list argument to this method, but the exception is going to be new ArgumentNullException("list"). The ExceptionArgument.list should be changed to ExceptionArgument.collection.

  • Fixed

#23166 (comment)

This exception message is "Index must be within the bounds of the List.". The code consuming this isn't using "List", though.

  • Fixed

#23166 (comment)
#23166 (comment)

The exception message here is "Offset and length were out of bounds for the array or count is greater than the number of elements from index to the end of the source collection"... but the arguments to this method are "index" and "count", not "offset" and "length"... are we ok with that mismatch?

  • Not Fixed

I don't think is appropriate to adjust the parameters or code as recommended. As List.cs uses the exact same exception and we wrote our code to follow the same standard as List.cs which is what we have been doing while we work through this change. @stephentoub if this is not correct, please advise on direction.

coreclr/src/System.Private.CoreLib/shared/System/Collections/Generic/List.cs

Lines 927 to 928 in b93891a

if (_size - index < count)
ThrowHelper.ThrowArgumentException(ExceptionResource.Argument_InvalidOffLen);

cc: @ahsonkhan, @safern, @justinvp

safern
safern reviewed Mar 18, 2019
View reviewed changes
Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

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

other than @stephentoub's comment, LGTM

Updated exception handling to throw ThrowHelper.ThrowArgumentOutOfRan…
a407608 
…ge_IndexException() when the ExceptionArgument was 'Index'
stephentoub
stephentoub approved these changes Mar 22, 2019
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Thanks.

@stephentoub stephentoub merged commit 01adae8 into dotnet:master Mar 22, 2019
Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/corefx that referenced this pull request Mar 22, 2019
@dotnet-bot
Updated Exception Handling for Collection<T> (dotnet/coreclr#23290)
df74889 
* Updated Argument Helper param from list->collection since the parameter name is collection

* Updated exception message to use an out of range exception that doesn't explicitly reference a list

* Simplified if statements that verify if the index is out of range

* Updated if logic to be simplified using (uint)

* Updated exception handling to throw ThrowHelper.ThrowArgumentOutOfRange_IndexException() when the ExceptionArgument was 'Index'

Signed-off-by: dotnet-bot <[email protected]>
Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/corert that referenced this pull request Mar 22, 2019
@dotnet-bot
Updated Exception Handling for Collection<T> (dotnet/coreclr#23290)
0736e89 
* Updated Argument Helper param from list->collection since the parameter name is collection

* Updated exception message to use an out of range exception that doesn't explicitly reference a list

* Simplified if statements that verify if the index is out of range

* Updated if logic to be simplified using (uint)

* Updated exception handling to throw ThrowHelper.ThrowArgumentOutOfRange_IndexException() when the ExceptionArgument was 'Index'

Signed-off-by: dotnet-bot <[email protected]>
jkotas pushed a commit to dotnet/corert that referenced this pull request Mar 22, 2019
@jkotas
Updated Exception Handling for Collection<T> (dotnet/coreclr#23290)
97709bf 
* Updated Argument Helper param from list->collection since the parameter name is collection

* Updated exception message to use an out of range exception that doesn't explicitly reference a list

* Simplified if statements that verify if the index is out of range

* Updated if logic to be simplified using (uint)

* Updated exception handling to throw ThrowHelper.ThrowArgumentOutOfRange_IndexException() when the ExceptionArgument was 'Index'

Signed-off-by: dotnet-bot <[email protected]>
jkotas pushed a commit to dotnet/corefx that referenced this pull request Mar 22, 2019
@jkotas
Updated Exception Handling for Collection<T> (dotnet/coreclr#23290)
c797d0c 
* Updated Argument Helper param from list->collection since the parameter name is collection

* Updated exception message to use an out of range exception that doesn't explicitly reference a list

* Simplified if statements that verify if the index is out of range

* Updated if logic to be simplified using (uint)

* Updated exception handling to throw ThrowHelper.ThrowArgumentOutOfRange_IndexException() when the ExceptionArgument was 'Index'

Signed-off-by: dotnet-bot <[email protected]>
Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/mono that referenced this pull request Mar 22, 2019
@dotnet-bot
Updated Exception Handling for Collection<T> (dotnet/coreclr#23290)
f998472 
* Updated Argument Helper param from list->collection since the parameter name is collection

* Updated exception message to use an out of range exception that doesn't explicitly reference a list

* Simplified if statements that verify if the index is out of range

* Updated if logic to be simplified using (uint)

* Updated exception handling to throw ThrowHelper.ThrowArgumentOutOfRange_IndexException() when the ExceptionArgument was 'Index'

Signed-off-by: dotnet-bot <[email protected]>
marek-safar pushed a commit to mono/mono that referenced this pull request Mar 22, 2019
@marek-safar
Updated Exception Handling for Collection<T> (dotnet/coreclr#23290)
8fe79b4 
* Updated Argument Helper param from list->collection since the parameter name is collection

* Updated exception message to use an out of range exception that doesn't explicitly reference a list

* Simplified if statements that verify if the index is out of range

* Updated if logic to be simplified using (uint)

* Updated exception handling to throw ThrowHelper.ThrowArgumentOutOfRange_IndexException() when the ExceptionArgument was 'Index'

Signed-off-by: dotnet-bot <[email protected]>
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
@stephentoub
Updated Exception Handling for Collection<T> (dotnet/coreclr#23290)
59953ff 
* Updated Argument Helper param from list->collection since the parameter name is collection

* Updated exception message to use an out of range exception that doesn't explicitly reference a list

* Simplified if statements that verify if the index is out of range

* Updated if logic to be simplified using (uint)

* Updated exception handling to throw ThrowHelper.ThrowArgumentOutOfRange_IndexException() when the ExceptionArgument was 'Index'


Commit migrated from dotnet/coreclr@01adae8
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@safern safern safern left review comments

@jkotas jkotas jkotas approved these changes

@stephentoub stephentoub stephentoub approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@SkyeHoefling @stephentoub @jkotas @safern