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

Add ExtractToImmutable #180

Merged
jaredpar merged 8 commits into from
Jan 9, 2015
Merged

Add ExtractToImmutable #180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
61f778d
Added ExtractToImmutable
jaredpar Dec 3, 2014
101e03d
Respond to PR feedback
jaredpar Dec 3, 2014
0be9fb3
Respond to PR feedback
jaredpar Dec 3, 2014
f51ba88
Fix up bad rebase merge
jaredpar Jan 6, 2015
e034c93
Changed name to MoveToImmutable
jaredpar Jan 7, 2015
c743cfb
Make Builder more like List<T>
jaredpar Jan 7, 2015
dd2b6ea
Added some more tests
jaredpar Jan 9, 2015
d4f998e
Respond to PR feedback
jaredpar Jan 9, 2015
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
21 changes: 20 additions & 1 deletion src/System.Collections.Immutable/src/Strings.Designer.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions src/System.Collections.Immutable/src/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,12 @@
<data name="CannotFindOldValue" xml:space="preserve">
<value>Cannot find the old value</value>
</data>
<data name="CapacityMustBeGreaterThanOrEqualToCount" xml:space="preserve">
<value>Capacity was less than the current Count of elements.</value>
</data>
<data name="CapacityMustEqualCountOnMove" xml:space="preserve">
<value>MoveToImmutable can only be performed when Count equals Capacity.</value>
</data>
<data name="CollectionModifiedDuringEnumeration" xml:space="preserve">
<value>Collection was modified; enumeration operation may not execute.</value>
</data>
Expand Down
143 changes: 79 additions & 64 deletions ...System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray`1+Builder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ public sealed class Builder : IList<T>, IReadOnlyList<T>
/// <summary>
/// The backing array for the builder.
/// </summary>
private RefAsValueType<T>[] _elements;
private T[] _elements;

/// <summary>
/// The number of initialized elements in the array.
Expand All @@ -36,8 +36,8 @@ public sealed class Builder : IList<T>, IReadOnlyList<T>
internal Builder(int capacity)
{
Requires.Range(capacity >= 0, "capacity");
_elements = new RefAsValueType<T>[capacity];
this.Count = 0;
_elements = new T[capacity];
_count = 0;
}

/// <summary>
Expand All @@ -49,7 +49,41 @@ internal Builder()
}

/// <summary>
/// Gets or sets the length of the array.
/// Get and sets the length of the internal array. When set the internal array is
/// is reallocated to the given capacity if it is not already the specified length.
/// </summary>
public int Capacity
{
get { return _elements.Length; }
set
Copy link
Copy Markdown

@sharwell sharwell Jan 9, 2015

Choose a reason for hiding this comment

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

Why not just use the following setter?

set
{
    if (value < _count)
    {
        throw new ArgumentException(Strings.CapacityMustBeGreaterThanOrEqualToCount, paramName: "value");
    }

    Array.Resize(ref _elements, value);
}

Copy link
Copy Markdown
Member Author

@jaredpar jaredpar Jan 9, 2015

Choose a reason for hiding this comment

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

In the case of value == 0 this would add an extra allocation.

Copy link
Copy Markdown

@sharwell sharwell Jan 9, 2015

Choose a reason for hiding this comment

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

Actually it wouldn't. I missed this on my first read too. Due to the argument validation, the only way value==0 is allowed is when _elements.Length == 0. When that occurs Array.Resize is a NOP because the current length and requested length are the same.

Copy link
Copy Markdown
Member Author

@jaredpar jaredpar Jan 9, 2015

Choose a reason for hiding this comment

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

@sharwell true.

This particular implementation was basically taken from List<T>::set_Capacity. Given it's high use within .Net I error'd in just taking that implementation over devising my own.

Copy link
Copy Markdown

@sharwell sharwell Jan 9, 2015

Choose a reason for hiding this comment

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

The implementation for List<T>.Capacity was taken from ArrayList.Capacity during the development of .NET 2.0. At the time that property was added, it's unlikely that the Array.Resize<T> method existed, hence why it was not used there.

Considering how much simpler my proposed implementation is, I recommend at least giving it full consideration since it would improve the overall size and readability of the code with (from what I've seen so far) no downside.

{
if (value < _count)
{
throw new ArgumentException(Strings.CapacityMustBeGreaterThanOrEqualToCount, paramName: "value");
}

if (value != _elements.Length)
{
if (value > 0)
Copy link
Copy Markdown

@sharwell sharwell Jan 9, 2015

Choose a reason for hiding this comment

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

Why not just use the following for this block? EDIT: This is suboptimal

if (value > 0)
{
    Array.Resize(ref _elements, value);
}

{
var temp = new T[value];
if (_count > 0)
{
Array.Copy(_elements, 0, temp, 0, _count);
}

_elements = temp;
}
else
{
_elements = ImmutableArray<T>.Empty.array;
}
}
}
}

/// <summary>
/// Gets or sets the length of the builder.
/// </summary>
/// <remarks>
/// If the value is decreased, the array contents are truncated.
Expand Down Expand Up @@ -81,7 +115,7 @@ public int Count
{
for (int i = value; i < this.Count; i++)
{
_elements[i].Value = default(T);
_elements[i] = default(T);
}
}
}
Expand Down Expand Up @@ -111,7 +145,7 @@ public T this[int index]
throw new IndexOutOfRangeException();
}

return _elements[index].Value;
return _elements[index];
}

set
Expand All @@ -121,7 +155,7 @@ public T this[int index]
throw new IndexOutOfRangeException();
}

_elements[index].Value = value;
_elements[index] = value;
}
}

Expand All @@ -141,14 +175,33 @@ bool ICollection<T>.IsReadOnly
/// <returns>An immutable array.</returns>
public ImmutableArray<T> ToImmutable()
{
if (this.Count == 0)
if (Count == 0)
{
return Empty;
}

return new ImmutableArray<T>(this.ToArray());
}

/// <summary>
/// Extracts the internal array as an <see cref="ImmutableArray{T}"/> and replaces it
/// with a zero length array.
/// </summary>
/// <exception cref="InvalidOperationException">When <see cref="ImmutableArray{T}.Builder.Count"/> doesn't
/// equal <see cref="ImmutableArray{T}.Builder.Capacity"/>.</exception>
public ImmutableArray<T> MoveToImmutable()
{
if (Capacity != Count)
{
throw new InvalidOperationException(Strings.CapacityMustEqualCountOnMove);
}

T[] temp = _elements;
_elements = ImmutableArray<T>.Empty.array;
Copy link
Copy Markdown
Member

@weshaggard weshaggard Jan 9, 2015

Choose a reason for hiding this comment

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

Can you please explicitly type temp here to T[]?

_count = 0;
return new ImmutableArray<T>(temp);
}

/// <summary>
/// Removes all items from the <see cref="T:System.Collections.Generic.ICollection`1" />.
/// </summary>
Expand All @@ -173,7 +226,7 @@ public void Insert(int index, T item)
}

_count++;
_elements[index].Value = item;
_elements[index] = item;
}

/// <summary>
Expand All @@ -183,7 +236,7 @@ public void Insert(int index, T item)
public void Add(T item)
{
this.EnsureCapacity(this.Count + 1);
_elements[_count++].Value = item;
_elements[_count++] = item;
}

/// <summary>
Expand Down Expand Up @@ -220,7 +273,7 @@ public void AddRange(params T[] items)
var nodes = _elements;
for (int i = 0; i < items.Length; i++)
{
nodes[offset + i].Value = items[i];
nodes[offset + i] = items[i];
}
}

Expand All @@ -238,7 +291,7 @@ public void AddRange<TDerived>(TDerived[] items) where TDerived : T
var nodes = _elements;
for (int i = 0; i < items.Length; i++)
{
nodes[offset + i].Value = items[i];
nodes[offset + i] = items[i];
}
}

Expand All @@ -258,7 +311,7 @@ public void AddRange(T[] items, int length)
var nodes = _elements;
for (int i = 0; i < length; i++)
{
nodes[offset + i].Value = items[i];
nodes[offset + i] = items[i];
}
}

Expand Down Expand Up @@ -372,7 +425,7 @@ public T[] ToArray()
var elements = _elements;
for (int i = 0; i < tmp.Length; i++)
{
tmp[i] = elements[i].Value;
tmp[i] = elements[i];
}

return tmp;
Expand All @@ -387,18 +440,14 @@ public void CopyTo(T[] array, int index)
{
Requires.NotNull(array, "array");
Requires.Range(index >= 0 && index + this.Count <= array.Length, "start");

foreach (var item in this)
{
array[index++] = item;
}
Array.Copy(_elements, 0, array, index, this.Count);
}

/// <summary>
/// Resizes the array to accommodate the specified capacity requirement.
/// </summary>
/// <param name="capacity">The required capacity.</param>
public void EnsureCapacity(int capacity)
private void EnsureCapacity(int capacity)
{
if (_elements.Length < capacity)
{
Expand Down Expand Up @@ -468,13 +517,13 @@ public int IndexOf(T item, int startIndex, int count, IEqualityComparer<T> equal

if (equalityComparer == EqualityComparer<T>.Default)
{
return Array.IndexOf(_elements, new RefAsValueType<T>(item), startIndex, count);
return Array.IndexOf(_elements, item, startIndex, count);
}
else
{
for (int i = startIndex; i < startIndex + count; i++)
{
if (equalityComparer.Equals(_elements[i].Value, item))
if (equalityComparer.Equals(_elements[i], item))
{
return i;
}
Expand Down Expand Up @@ -555,13 +604,13 @@ public int LastIndexOf(T item, int startIndex, int count, IEqualityComparer<T> e

if (equalityComparer == EqualityComparer<T>.Default)
{
return Array.LastIndexOf(_elements, new RefAsValueType<T>(item), startIndex, count);
return Array.LastIndexOf(_elements, item, startIndex, count);
}
else
{
for (int i = startIndex; i >= startIndex - count + 1; i--)
{
if (equalityComparer.Equals(item, _elements[i].Value))
if (equalityComparer.Equals(item, _elements[i]))
{
return i;
}
Expand All @@ -576,13 +625,7 @@ public int LastIndexOf(T item, int startIndex, int count, IEqualityComparer<T> e
/// </summary>
public void ReverseContents()
Copy link
Copy Markdown
Member

@KrzysztofCwalina KrzysztofCwalina Dec 4, 2014

Choose a reason for hiding this comment

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

I would just call it reverse to be consistent with List.

{
int end = this.Count - 1;
for (int i = 0, j = end; i < j; i++, j--)
{
var tmp = _elements[i].Value;
_elements[i] = _elements[j];
_elements[j].Value = tmp;
}
Array.Reverse(_elements, 0, _count);
}

/// <summary>
Expand All @@ -592,7 +635,7 @@ public void Sort()
{
if (Count > 1)
{
Array.Sort(_elements, 0, this.Count, Comparer.Default);
Array.Sort(_elements, 0, this.Count, Comparer<T>.Default);
}
}

Expand All @@ -604,7 +647,7 @@ public void Sort(IComparer<T> comparer)
{
if (Count > 1)
Copy link
Copy Markdown
Member

@stephentoub stephentoub Dec 3, 2014

Choose a reason for hiding this comment

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

IIRC, these Count > 1 checks were previously added to avoid allocating the Comparer wrapper. Now that the wrapper isn't needed, I wonder if the checks are still beneficial.

{
Array.Sort(_elements, 0, this.Count, Comparer.Create(comparer));
Array.Sort(_elements, 0, _count, comparer);
}
}

Expand All @@ -623,7 +666,7 @@ public void Sort(int index, int count, IComparer<T> comparer)

if (count > 1)
{
Array.Sort(_elements, index, count, Comparer.Create(comparer));
Array.Sort(_elements, index, count, comparer);
}
}

Expand Down Expand Up @@ -663,7 +706,7 @@ IEnumerator IEnumerable.GetEnumerator()
/// <typeparam name="TDerived">The type of source elements.</typeparam>
/// <param name="items">The source array.</param>
/// <param name="length">The number of elements to add to this array.</param>
private void AddRange<TDerived>(RefAsValueType<TDerived>[] items, int length) where TDerived : T
private void AddRange<TDerived>(TDerived[] items, int length) where TDerived : T
{
this.EnsureCapacity(this.Count + length);

Expand All @@ -673,35 +716,7 @@ private void AddRange<TDerived>(RefAsValueType<TDerived>[] items, int length) wh
var nodes = _elements;
for (int i = 0; i < length; i++)
{
nodes[offset + i].Value = items[i].Value;
}
}

private sealed class Comparer : IComparer<RefAsValueType<T>>
{
private readonly IComparer<T> _comparer;

public static readonly Comparer Default = new Comparer(Comparer<T>.Default);

public static Comparer Create(IComparer<T> comparer)
{
if (comparer == null || comparer == Comparer<T>.Default)
{
return Default;
}

return new Comparer(comparer);
}

private Comparer(IComparer<T> comparer)
{
Requires.NotNull(comparer, "comparer"); // use Comparer.Default instead of passing null
_comparer = comparer;
}

public int Compare(RefAsValueType<T> x, RefAsValueType<T> y)
{
return _comparer.Compare(x.Value, y.Value);
nodes[offset + i] = items[i];
}
}
}
Expand Down
Loading