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

Implement slicing APIs for ArraySegment<T> #9926

Merged
merged 8 commits into from
Apr 10, 2017
Merged

Implement slicing APIs for ArraySegment<T> #9926

Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2c034d4
Add slicing APIs to ArraySegment
jamesqo Mar 3, 2017
2d26de8
Implement Slice
jamesqo Mar 3, 2017
8a5c436
Respond to PR feedback
jamesqo Mar 4, 2017
d9935bc
Respond to PR feedback
jamesqo Mar 13, 2017
c6775c8
Respond to PR feedback
jamesqo Mar 21, 2017
726c86f
Merge branch 'aseg.slice' of github.com:jamesqo/coreclr into aseg.slice
jamesqo Apr 9, 2017
1e9c6e5
Disallow negative indices in Slice()
jamesqo Apr 9, 2017
e870fc0
Remove redundant explicit implementation
jamesqo Apr 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 86 additions & 24 deletions src/mscorlib/src/System/ArraySegment.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ namespace System
[Serializable]
public struct ArraySegment<T> : IList<T>, IReadOnlyList<T>
{
// Do not replace the array allocation with Array.Empty. We don't want to have the overhead of
// instantiating another generic type in addition to ArraySegment<T> for new type parameters.
public static ArraySegment<T> Empty { get; } = new ArraySegment<T>(new T[0]);

private readonly T[] _array;
private readonly int _offset;
private readonly int _count;
Expand Down Expand Up @@ -106,12 +110,31 @@ public int Count
}
}

public Enumerator GetEnumerator()
public T this[int index]
Copy link
Member

Choose a reason for hiding this comment

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

Have we already discussed the potential for this returning ref T instead of T? e.g.

public ref T this[int index]
{
    get
    {
        if ((uint)index >= (uint)_count) ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
        return ref _array[_offset + index];
    }
}

?

Copy link
Member

Choose a reason for hiding this comment

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

+1

This is why ref returns were invented. Definitely should be used here because there are no compat issues to consider on this new API.

Copy link
Member

@jkotas jkotas Apr 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

ref T returning indexer will not work well with array covariance. The rest of the ArraySegment is compatible with array covariance, so it will look odd.

String[] arr = new String[] { ... };
ArraySegment<object> segment = new ArraySegment(arr);
Object elem = segment[0]; // Throws exception if the indexer returns `ref T`

Copy link
Member

Choose a reason for hiding this comment

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

Ugh.

Copy link
Member

Choose a reason for hiding this comment

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

Arrays.....

{
if (_array == null)
ThrowHelper.ThrowInvalidOperationException(ExceptionResource.InvalidOperation_NullArray);
Contract.EndContractBlock();
get
{
if ((uint)index >= (uint)_count)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}

return _array[_offset + index];
}
set
{
if ((uint)index >= (uint)_count)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}

_array[_offset + index] = value;
}
}

public Enumerator GetEnumerator()
{
ThrowInvalidOperationIfDefault();
return new Enumerator(this);
}

Expand All @@ -132,6 +155,21 @@ public override int GetHashCode()
return hash;
}

public void CopyTo(T[] destination) => CopyTo(destination, 0);

public void CopyTo(T[] destination, int destinationIndex)
{
ThrowInvalidOperationIfDefault();
System.Array.Copy(_array, _offset, destination, destinationIndex, _count);
}

public void CopyTo(ArraySegment<T> destination)
{
ThrowInvalidOperationIfDefault();
destination.ThrowInvalidOperationIfDefault();
System.Array.Copy(_array, _offset, destination._array, destination._offset, _count);
Copy link
Member

Choose a reason for hiding this comment

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

This can write beyond limits of the destination

Copy link
Author

Choose a reason for hiding this comment

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

@jkotas Good catch. Fixed

}

public override bool Equals(Object obj)
{
if (obj is ArraySegment<T>)
Expand All @@ -145,6 +183,34 @@ public bool Equals(ArraySegment<T> obj)
return obj._array == _array && obj._offset == _offset && obj._count == _count;
}

public ArraySegment<T> Slice(int index)
Copy link
Member

Choose a reason for hiding this comment

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

C# team is considering adding slicing syntax. It's not yet clear what would libraries have to do to support it. It could be Slice(int) method (just like here) or Slice(Range) method.

When this C# syntax ships, it would be great if ArraySegment supported it.

@jaredpar, @MadsTorgersen, any word on the syntax? Shoudl we just assume the Range solution? Should we add Range struct to the platform?

Copy link
Author

Choose a reason for hiding this comment

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

@KrzysztofCwalina Is this blocking this PR? In the case they decide to add a Range struct we can just add another overload in addition to these two.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's blocking.

{
ThrowInvalidOperationIfDefault();
// Note: `index` is allowed to be negative so the start of this ArraySegment can be moved backwards.
Copy link
Member

Choose a reason for hiding this comment

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

Was the negative index for slicing discussed anywhere?

Copy link
Author

Choose a reason for hiding this comment

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

@KrzysztofCwalina Do you have an opinion for allowing negative indices for slicing?

Copy link
Author

Choose a reason for hiding this comment

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

@terrajobst Do you have an opinion on this either? (This is blocking the APIs from being added.)

Copy link
Member

Choose a reason for hiding this comment

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

@jkotas @KrzysztofCwalina how well do negative bounds work for our optimized bounds checking pattern?

if ((uint)index >= (uint)_count) ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);

I think we want to make sure indexing is blazing fast, so I don't think we want to allow negative indices as it would cause additional branches. In slicing it might be OK performance wise, but would feel inconsistent if I can't do it for regular indexing.

It's also a pattern that doesn't exist in the BCL. I don't think we should introduce it here.

Copy link
Member

Choose a reason for hiding this comment

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

I think we want to make sure indexing is blazing fast

Plain array is blazing fast. ArraySegment is not. It has extra overhead by design.

It's also a pattern that doesn't exist in the BCL. I don't think we should introduce it here.

Agree.

Copy link
Author

Choose a reason for hiding this comment

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

Updated PR to disallow negative indices.

return new ArraySegment<T>(_array, _offset + index, _count - index);
}

public ArraySegment<T> Slice(int index, int count)
{
ThrowInvalidOperationIfDefault();
// Note: `index` is allowed to be negative so the start of this ArraySegment can be moved backwards.
return new ArraySegment<T>(_array, _offset + index, count);
}

public T[] ToArray()
{
ThrowInvalidOperationIfDefault();

if (_count == 0)
{
return Empty._array;
}

var array = new T[_count];
System.Array.Copy(_array, _offset, array, 0, _count);
return array;
}

public static bool operator ==(ArraySegment<T> a, ArraySegment<T> b)
{
return a.Equals(b);
Expand All @@ -155,13 +221,14 @@ public bool Equals(ArraySegment<T> obj)
return !(a == b);
}

public static implicit operator ArraySegment<T>(T[] array) => new ArraySegment<T>(array);

#region IList<T>
T IList<T>.this[int index]
{
get
{
if (_array == null)
ThrowHelper.ThrowInvalidOperationException(ExceptionResource.InvalidOperation_NullArray);
ThrowInvalidOperationIfDefault();
if (index < 0 || index >= _count)
ThrowHelper.ThrowArgumentOutOfRange_IndexException();
Contract.EndContractBlock();
Expand All @@ -171,8 +238,7 @@ T IList<T>.this[int index]

set
{
if (_array == null)
ThrowHelper.ThrowInvalidOperationException(ExceptionResource.InvalidOperation_NullArray);
ThrowInvalidOperationIfDefault();
if (index < 0 || index >= _count)
ThrowHelper.ThrowArgumentOutOfRange_IndexException();
Contract.EndContractBlock();
Expand All @@ -183,9 +249,7 @@ T IList<T>.this[int index]

int IList<T>.IndexOf(T item)
{
if (_array == null)
ThrowHelper.ThrowInvalidOperationException(ExceptionResource.InvalidOperation_NullArray);
Contract.EndContractBlock();
ThrowInvalidOperationIfDefault();

int index = System.Array.IndexOf<T>(_array, item, _offset, _count);

Expand All @@ -211,8 +275,7 @@ T IReadOnlyList<T>.this[int index]
{
get
{
if (_array == null)
ThrowHelper.ThrowInvalidOperationException(ExceptionResource.InvalidOperation_NullArray);
ThrowInvalidOperationIfDefault();
if (index < 0 || index >= _count)
ThrowHelper.ThrowArgumentOutOfRange_IndexException();
Contract.EndContractBlock();
Expand Down Expand Up @@ -245,9 +308,7 @@ void ICollection<T>.Clear()

bool ICollection<T>.Contains(T item)
{
if (_array == null)
ThrowHelper.ThrowInvalidOperationException(ExceptionResource.InvalidOperation_NullArray);
Contract.EndContractBlock();
ThrowInvalidOperationIfDefault();

int index = System.Array.IndexOf<T>(_array, item, _offset, _count);

Expand All @@ -257,14 +318,7 @@ bool ICollection<T>.Contains(T item)
return index >= 0;
}

void ICollection<T>.CopyTo(T[] array, int arrayIndex)
{
if (_array == null)
ThrowHelper.ThrowInvalidOperationException(ExceptionResource.InvalidOperation_NullArray);
Contract.EndContractBlock();

System.Array.Copy(_array, _offset, array, arrayIndex, _count);
}
void ICollection<T>.CopyTo(T[] array, int arrayIndex) => CopyTo(array, arrayIndex);

bool ICollection<T>.Remove(T item)
{
Expand All @@ -283,6 +337,14 @@ bool ICollection<T>.Remove(T item)
IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
#endregion

private void ThrowInvalidOperationIfDefault()
{
if (_array == null)
{
ThrowHelper.ThrowInvalidOperationException(ExceptionResource.InvalidOperation_NullArray);
}
}

[Serializable]
public struct Enumerator : IEnumerator<T>
{
Expand Down