Implement generic interfaces on Regex collections #1756
Conversation
| { | ||
| if (array == null) | ||
| throw new ArgumentNullException("array"); | ||
| if (arrayIndex < 0 || arrayIndex > array.Length) |
There was a problem hiding this comment.
shouldn't it be arrayIndex >= array.Length?
There was a problem hiding this comment.
I originally had it like that, but then the behavior differs from List<T> and Dictionary<TKey, TValue>, which throws ArgumentException in that case instead of ArgumentOutOfRangeException.
There was a problem hiding this comment.
There's actually even a behavioral difference between List<T>.CopyTo and Dictionary<TKey, TValue>.CopyTo.
The following tests pass:
ICollection<int> list = new List<int> { 0, 1 };
int[] listArray = new int[2];
Assert.Throws<ArgumentOutOfRangeException>(() => list.CopyTo(listArray, -1));
Assert.Throws<ArgumentException>(() => list.CopyTo(listArray, 2));
Assert.Throws<ArgumentException>(() => list.CopyTo(listArray, 3));
ICollection<KeyValuePair<int, int>> dictionary = new Dictionary<int, int> { { 0, 0 }, { 1, 1 } };
KeyValuePair<int, int>[] dictionaryArray = new KeyValuePair<int, int>[2];
Assert.Throws<ArgumentOutOfRangeException>(() => dictionary.CopyTo(dictionaryArray, -1));
Assert.Throws<ArgumentException>(() => dictionary.CopyTo(dictionaryArray, 2));
Assert.Throws<ArgumentOutOfRangeException>(() => dictionary.CopyTo(dictionaryArray, 3));Notice the last case is ArgumentException for List<T> but ArgumentOutOfRangeException for Dictionary<TKey, TValue>
Right now CaptureCollection and GroupCollection are consistent with Dictionary<TKey, TValue> whereas MatchCollection is consistent with List<T> (because MatchCollection uses List<T>.CopyTo internally).
Maybe it'd be better if I remove the arrayIndex > array.Length check altogether from CaptureCollection and GroupCollection, so the exceptions are consistent with MatchCollection and List<T>. Or not worry about it too much because it's all ArgumentException anyway?
There was a problem hiding this comment.
Since we are already not very consistent, I would do either what you suggested (to simplify the code) or what I suggested (follow current convention and implemented correctly). I would not try to be consistent with incorrectly implemented range checks ;-)
Moreover, it would be great to have a helper for common range checks, so that people don't make similar mistakes (to those made in the existing collections). Something that can be called along the lines of:
Precondition.RequireFit(source : this, destination : array, destinationIndex : arrayIndex);
There was a problem hiding this comment.
After investigating how other collections implement CopyTo, I think we should leave the range checks in the PR as-is as this is how other collections implement CopyTo.
For example, all of the following cases do not throw:
new int[0].CopyTo(new int[10], 10);
new List<int>().CopyTo(new int[10], 10);
new LinkedList<int>().CopyTo(new int[10], 10);
new Queue<int>().CopyTo(new int[10], 10);
new Stack<int>().CopyTo(new int[10], 10);
new HashSet<int>().CopyTo(new int[10], 10);
new SortedSet<int>().CopyTo(new int[10], 10);
((ICollection<KeyValuePair<int, int>>)new Dictionary<int, int>()).CopyTo(new KeyValuePair<int,int>[10], 10);
((ICollection<KeyValuePair<int, int>>)new SortedDictionary<int, int>()).CopyTo(new KeyValuePair<int, int>[10], 10);
new ConcurrentStack<int>().CopyTo(new int[10], 10);
new ConcurrentQueue<int>().CopyTo(new int[10], 10);
new ConcurrentBag<int>().CopyTo(new int[10], 10);
((ICollection<KeyValuePair<int, int>>)new ConcurrentDictionary<int, int>()).CopyTo(new KeyValuePair<int, int>[10], 10);
ImmutableArray.Create<int>().CopyTo(new int[10], 10);
ImmutableList.Create<int>().CopyTo(new int[10], 10);
((ICollection<int>)ImmutableHashSet.Create<int>()).CopyTo(new int[10], 10);
((ICollection<int>)ImmutableSortedSet.Create<int>()).CopyTo(new int[10], 10);
((ICollection<KeyValuePair<int, int>>)ImmutableDictionary.Create<int, int>()).CopyTo(new KeyValuePair<int, int>[10], 10);
((ICollection<KeyValuePair<int, int>>)ImmutableSortedDictionary.Create<int, int>()).CopyTo(new KeyValuePair<int, int>[10], 10);If I change the range check from arrayIndex > array.Length to arrayIndex >= array.Length, then the regex collections would throw in such cases and be inconsistent with all of these collections.
|
@dotnet-bot test this please |
|
@weshaggard / @mmitche, it seems like all PRs against the future branch are failing in CI, I believe for issues that were already fixed in master, and I'm wondering if merging master to future would help...? |
|
@justinvp, master was just merged to future, in part because there were some fixes in master from a while ago that I believe were causing your PR's CIs to fail. There are now merge conflicts with your change. Can you fix it up appropriately and recommit/push? |
justinvp
force-pushed
the
regex_generic_collections
branch
2 times, most recently
from
2043924
to
3664fc9
Compare
|
Fixed. The conflict was due to the move from After running a build, the After the changes to
I'm not sure where in the build log to look for the reference conflicts it is warning about, or why it's warning about them. I see the same warning for System.Security.Principal.Windows.Tests.csproj when running
|
|
|
||
| bool ICollection<Capture>.Contains(Capture item) | ||
| { | ||
| var comparer = EqualityComparer<Capture>.Default; |
There was a problem hiding this comment.
Rather than duplicate logic, did you consider just having the implementation be:
return ((IList<Capture>)this).IndexOf(item) >= 0;?
|
A few minor comments, but otherwise LGTM. Thanks for doing this, Justin. |
Implement IList<T>, IReadOnlyList<T>, and IList on the Regex collections.
justinvp
force-pushed
the
regex_generic_collections
branch
from
3664fc9
to
9a764c6
Compare
|
I updated the PR to include Thanks for the review, @KrzysztofCwalina and @stephentoub! |
|
Yeah. Thanks for the contribution. I will start new rounds of tests and merge when they pass. |
|
@dotnet-bot test this please |
|
Unrelated test failure on linux:
Looks like https://github.com/dotnet/corefx/issues/1904 #1905 needs to be merged into future. |
Yup, that should fix it. |
|
@dotnet-bot test this please |
|
@justinvp, the future branch is now up-to-date with master. |
|
@stephentoub, thanks! Does @dotnet-bot accept commands from me?... |
|
@dotnet-bot test this please |
|
Apparently 😄 |
|
@dotnet-bot test this please |
|
Thanks for the contribution, Justin! |
Implement generic interfaces on Regex collections
Implement
IList<T>,IReadOnlyList<T>, andIListon the Regex collections.Fixes #271