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

Collection<T>: Validate parameters in the public methods#23166

Merged

stephentoub merged 1 commit intodotnet:masterfrom

justinvp:collection_range

Mar 15, 2019

justinvp commented Mar 10, 2019 •

edited

Loading

I was looking over the new Range methods on Collection<T> (added in #23018) and it occurred to me that for all existing methods, parameter validation is done in the public methods before calling the protected virtual methods. This changes the new Range methods to do the same.

cc: @ahoefling, @ahsonkhan, @safern


          Collection<T>: Validate parameters in the public methods

5806b6c

I was looking over the new Range methods on `Collection<T>` and it occurred to me that for all existing methods, parameter validation is done in the *public* methods before calling the *protected virtual* methods. This changes the new Range methods to do the same.

Member

filipnavara commented Mar 10, 2019 •

edited

Loading

Sounds like a good idea for a couple of reasons:

People overriding the methods don't need to perform the error checking themselves. If someone already did that then it won't break existing code. The parameter checks look inexpensive and quite generic.
The public method may get inlined and the checks could be eliminated altogether for the particular call site.

SkyeHoefling commented Mar 11, 2019

I agree, this will solve some problems we are already having in the CoreFX project.

SkyeHoefling mentioned this pull request

Added Range Manipulation APIs to Collection<T> and ObservableCollection<T> dotnet/corefx#35772

Merged

ahsonkhan added the area-System.Collections label

ahsonkhan added this to the 3.0 milestone

stephentoub reviewed

View reviewed changes

src/System.Private.CoreLib/shared/System/Collections/ObjectModel/Collection.cs

+                          if (collection == null)
+                          {
+                              ThrowHelper.ThrowArgumentNullException(ExceptionArgument.list);

Member

stephentoub Mar 15, 2019

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.

stephentoub reviewed

View reviewed changes

src/System.Private.CoreLib/shared/System/Collections/ObjectModel/Collection.cs

+                          if ((uint)index > (uint)items.Count)
+                          {
+                              ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index, ExceptionResource.ArgumentOutOfRange_ListInsert);

Member

stephentoub Mar 15, 2019

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

stephentoub reviewed

View reviewed changes

src/System.Private.CoreLib/shared/System/Collections/ObjectModel/Collection.cs

+                          if (index > items.Count - count)
+                          {
+                              ThrowHelper.ThrowArgumentException(ExceptionResource.Argument_InvalidOffLen);

Member

stephentoub Mar 15, 2019

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?

stephentoub reviewed

View reviewed changes

src/System.Private.CoreLib/shared/System/Collections/ObjectModel/Collection.cs

+                          if (index > items.Count - count)
+                          {
+                              ThrowHelper.ThrowArgumentException(ExceptionResource.Argument_InvalidOffLen);

Member

stephentoub Mar 15, 2019

Same comment as earlier.

stephentoub reviewed

View reviewed changes

src/System.Private.CoreLib/shared/System/Collections/ObjectModel/Collection.cs

+                          if (collection == null)
+                          {
+                              ThrowHelper.ThrowArgumentNullException(ExceptionArgument.list);

Member

stephentoub Mar 15, 2019

Same comment as earlier.

stephentoub approved these changes

View reviewed changes

Member

stephentoub left a comment

The actual changes here LGTM. But some of the previous code that's being moved is suspect, in particular with regards to the contents of the exceptions being thrown. Let's merge this, but then we should follow up to get those exceptions fixed as is appropriate.

stephentoub merged commit 5231179 into dotnet:master

Author

justinvp commented Mar 15, 2019

Let's merge this, but then we should follow up to get those exceptions fixed as is appropriate.

@ahoefling, are you planning to follow-up with a PR?

safern reviewed

View reviewed changes

Member

safern left a comment

LGTM, thanks @justinvp

SkyeHoefling commented Mar 15, 2019

@justinvp I have some time today and will work on a PR for this based on the feedback above

SkyeHoefling mentioned this pull request

Updated Exception Handling for Collection<T> #23290

Merged

justinvp mentioned this pull request

Collection<T>.AddRange should validate its parameters dotnet/runtime#12784

Closed

picenka21 pushed a commit to picenka21/runtime that referenced this pull request


          Collection<T>: Validate parameters in the public methods (dotnet/core…

6ed2db0

…clr#23166)

I was looking over the new Range methods on `Collection<T>` and it occurred to me that for all existing methods, parameter validation is done in the *public* methods before calling the *protected virtual* methods. This changes the new Range methods to do the same.

Commit migrated from dotnet/coreclr@5231179

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Collections