From 61f778dbaa2d0cc088004b8a277e5e48c5a8206e Mon Sep 17 00:00:00 2001 From: Jared Parsons Date: Tue, 2 Dec 2014 22:01:23 -0800 Subject: [PATCH 1/8] Added ExtractToImmutable This is an alternate implementation idea for having a zero copy ImmutableArray construction mechanism. This reuses the existing builder and adds an ExtractToImmutable method which wraps the internal array with an ImmutableArray and replaces it with an empty array. As a part of this change the RefAsValueType optimization is removed from the Builder. It's a requirement to have such a method because ImmutableArray wraps a T[] not a RefAsValueType[] --- .../Collections/Immutable/ImmutableArray.cs | 13 +++ .../Immutable/ImmutableArray`1+Builder.cs | 91 ++++++++++++------- .../tests/ImmutableArrayBuilderTest.cs | 38 ++++++++ 3 files changed, 110 insertions(+), 32 deletions(-) diff --git a/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray.cs b/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray.cs index 3faf23899716..ae54882950f5 100644 --- a/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray.cs +++ b/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray.cs @@ -396,6 +396,19 @@ public static ImmutableArray.Builder CreateBuilder(int initialCapacity) return new ImmutableArray.Builder(initialCapacity); } + /// + /// Initializes a new instance of the class with + /// the capacity and count set to the specified value. + /// + /// The type of elements stored in the array. + /// The count of the underlying initial backing array and builder. + /// A new builder. + [Pure] + public static ImmutableArray.Builder CreateBuilderWithCount(int count) + { + return new ImmutableArray.Builder(capacity: count, count: count); + } + /// /// Enumerates a sequence exactly once and produces an immutable array of its contents. /// diff --git a/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray`1+Builder.cs b/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray`1+Builder.cs index 69438b052e86..7463e9700408 100644 --- a/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray`1+Builder.cs +++ b/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray`1+Builder.cs @@ -22,7 +22,7 @@ public sealed class Builder : IList, IReadOnlyList /// /// The backing array for the builder. /// - private RefAsValueType[] _elements; + private T[] _elements; /// /// The number of initialized elements in the array. @@ -36,8 +36,21 @@ public sealed class Builder : IList, IReadOnlyList internal Builder(int capacity) { Requires.Range(capacity >= 0, "capacity"); - _elements = new RefAsValueType[capacity]; - this.Count = 0; + _elements = new T[capacity]; + _count = 0; + } + + /// + /// Initializes a new instance of the class. + /// + /// The initial capacity of the internal array. + /// The initial count of the builder. + internal Builder(int capacity, int count) + { + Requires.Range(capacity >= 0, "capacity"); + Requires.Range(count <= capacity, "count"); + _elements = new T[capacity]; + _count = count; } /// @@ -49,7 +62,15 @@ internal Builder() } /// - /// Gets or sets the length of the array. + /// Get the length of the internal array. + /// + public int Capacity + { + get { return _elements.Length; } + } + + /// + /// Gets or sets the length of the builder. /// /// /// If the value is decreased, the array contents are truncated. @@ -81,7 +102,7 @@ public int Count { for (int i = value; i < this.Count; i++) { - _elements[i].Value = default(T); + _elements[i] = default(T); } } } @@ -111,7 +132,7 @@ public T this[int index] throw new IndexOutOfRangeException(); } - return _elements[index].Value; + return _elements[index]; } set @@ -121,7 +142,7 @@ public T this[int index] throw new IndexOutOfRangeException(); } - _elements[index].Value = value; + _elements[index] = value; } } @@ -149,6 +170,18 @@ public ImmutableArray ToImmutable() return new ImmutableArray(this.ToArray()); } + /// + /// Extracts the internal array as an and replaces it + /// with a zero length array + /// + public ImmutableArray ExtractToImmutable() + { + var temp = _elements; + _elements = ImmutableArray.Empty.array; + _count = 0; + return new ImmutableArray(temp); + } + /// /// Removes all items from the . /// @@ -173,7 +206,7 @@ public void Insert(int index, T item) } _count++; - _elements[index].Value = item; + _elements[index] = item; } /// @@ -183,7 +216,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; } /// @@ -220,7 +253,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]; } } @@ -238,7 +271,7 @@ public void AddRange(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]; } } @@ -258,7 +291,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]; } } @@ -387,11 +420,7 @@ 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); } /// @@ -468,13 +497,13 @@ public int IndexOf(T item, int startIndex, int count, IEqualityComparer equal if (equalityComparer == EqualityComparer.Default) { - return Array.IndexOf(_elements, new RefAsValueType(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; } @@ -555,13 +584,13 @@ public int LastIndexOf(T item, int startIndex, int count, IEqualityComparer e if (equalityComparer == EqualityComparer.Default) { - return Array.LastIndexOf(_elements, new RefAsValueType(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; } @@ -576,13 +605,7 @@ public int LastIndexOf(T item, int startIndex, int count, IEqualityComparer e /// public void ReverseContents() { - 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); } /// @@ -592,7 +615,7 @@ public void Sort() { if (Count > 1) { - Array.Sort(_elements, 0, this.Count, Comparer.Default); + Array.Sort(_elements, 0, this.Count, Comparer.Default); } } @@ -604,7 +627,7 @@ public void Sort(IComparer comparer) { if (Count > 1) { - Array.Sort(_elements, 0, this.Count, Comparer.Create(comparer)); + Array.Sort(_elements, 0, _count, comparer); } } @@ -623,7 +646,7 @@ public void Sort(int index, int count, IComparer comparer) if (count > 1) { - Array.Sort(_elements, index, count, Comparer.Create(comparer)); + Array.Sort(_elements, index, count, comparer); } } @@ -663,7 +686,7 @@ IEnumerator IEnumerable.GetEnumerator() /// The type of source elements. /// The source array. /// The number of elements to add to this array. - private void AddRange(RefAsValueType[] items, int length) where TDerived : T + private void AddRange(TDerived[] items, int length) where TDerived : T { this.EnsureCapacity(this.Count + length); @@ -673,6 +696,7 @@ private void AddRange(RefAsValueType[] items, int length) wh var nodes = _elements; for (int i = 0; i < length; i++) { +<<<<<<< HEAD nodes[offset + i].Value = items[i].Value; } } @@ -702,6 +726,9 @@ private Comparer(IComparer comparer) public int Compare(RefAsValueType x, RefAsValueType y) { return _comparer.Compare(x.Value, y.Value); +======= + nodes[offset + i] = items[i]; +>>>>>>> Added ExtractToImmutable } } } diff --git a/src/System.Collections.Immutable/tests/ImmutableArrayBuilderTest.cs b/src/System.Collections.Immutable/tests/ImmutableArrayBuilderTest.cs index 5a850b6809cb..113b37a88abc 100644 --- a/src/System.Collections.Immutable/tests/ImmutableArrayBuilderTest.cs +++ b/src/System.Collections.Immutable/tests/ImmutableArrayBuilderTest.cs @@ -469,6 +469,44 @@ public void IEnumerator() Assert.False(enumerator.MoveNext()); } + [Fact] + public void ExtractToImmutableNormal() + { + var builder = ImmutableArray.CreateBuilderWithCount(2); + Assert.Equal(2, builder.Count); + Assert.Equal(2, builder.Capacity); + builder[1] = "b"; + builder[0] = "a"; + var array = builder.ExtractToImmutable(); + Assert.Equal(new[] { "a", "b" }, array); + Assert.Equal(0, builder.Count); + Assert.Equal(0, builder.Capacity); + } + + [Fact] + public void ExtractToImmutableRepeat() + { + var builder = ImmutableArray.CreateBuilderWithCount(2); + builder[0] = "a"; + builder[1] = "b"; + var array1 = builder.ExtractToImmutable(); + var array2 = builder.ExtractToImmutable(); + Assert.Equal(new[] { "a", "b" }, array1); + Assert.Equal(0, array2.Length); + } + + [Fact] + public void ExtractToImmutablePartialFill() + { + var builder = ImmutableArray.CreateBuilder(4); + builder.Add(42); + builder.Add(13); + Assert.Equal(4, builder.Capacity); + Assert.Equal(2, builder.Count); + var array = builder.ExtractToImmutable(); + Assert.Equal(new[] { 42, 13, 0, 0 }, array); + } + protected override IEnumerable GetEnumerableOf(params T[] contents) { var builder = new ImmutableArray.Builder(contents.Length); From 101e03d62c360539cd9cc85476985ae600f54640 Mon Sep 17 00:00:00 2001 From: Jared Parsons Date: Wed, 3 Dec 2014 13:14:23 -0800 Subject: [PATCH 2/8] Respond to PR feedback --- .../Immutable/ImmutableArray`1+Builder.cs | 34 +------------------ .../tests/ImmutableArrayBuilderTest.cs | 23 +++++++++++++ .../tests/ImmutableArrayTest.cs | 8 +++++ 3 files changed, 32 insertions(+), 33 deletions(-) diff --git a/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray`1+Builder.cs b/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray`1+Builder.cs index 7463e9700408..7a8a9db644d6 100644 --- a/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray`1+Builder.cs +++ b/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray`1+Builder.cs @@ -48,7 +48,7 @@ internal Builder(int capacity) internal Builder(int capacity, int count) { Requires.Range(capacity >= 0, "capacity"); - Requires.Range(count <= capacity, "count"); + Requires.Range(count >= 0 && count <= capacity, "count"); _elements = new T[capacity]; _count = count; } @@ -696,39 +696,7 @@ private void AddRange(TDerived[] items, int length) where TDerived : T var nodes = _elements; for (int i = 0; i < length; i++) { -<<<<<<< HEAD - nodes[offset + i].Value = items[i].Value; - } - } - - private sealed class Comparer : IComparer> - { - private readonly IComparer _comparer; - - public static readonly Comparer Default = new Comparer(Comparer.Default); - - public static Comparer Create(IComparer comparer) - { - if (comparer == null || comparer == Comparer.Default) - { - return Default; - } - - return new Comparer(comparer); - } - - private Comparer(IComparer comparer) - { - Requires.NotNull(comparer, "comparer"); // use Comparer.Default instead of passing null - _comparer = comparer; - } - - public int Compare(RefAsValueType x, RefAsValueType y) - { - return _comparer.Compare(x.Value, y.Value); -======= nodes[offset + i] = items[i]; ->>>>>>> Added ExtractToImmutable } } } diff --git a/src/System.Collections.Immutable/tests/ImmutableArrayBuilderTest.cs b/src/System.Collections.Immutable/tests/ImmutableArrayBuilderTest.cs index 113b37a88abc..2ae212f92e66 100644 --- a/src/System.Collections.Immutable/tests/ImmutableArrayBuilderTest.cs +++ b/src/System.Collections.Immutable/tests/ImmutableArrayBuilderTest.cs @@ -507,6 +507,29 @@ public void ExtractToImmutablePartialFill() Assert.Equal(new[] { 42, 13, 0, 0 }, array); } + [Fact] + public void ExtractToImmutableThenUse() + { + var builder = ImmutableArray.CreateBuilderWithCount(2); + Assert.Equal(2, builder.ExtractToImmutable().Length); + Assert.Equal(0, builder.Capacity); + builder.Add("a"); + builder.Add("b"); + Assert.Equal(2, builder.Count); + Assert.True(builder.Capacity >= 2); + Assert.Equal(new[] { "a", "b" }, builder.ExtractToImmutable()); + } + + [Fact] + public void ExtractToImmutableAfterClear() + { + var builder = ImmutableArray.CreateBuilderWithCount(2); + builder[0] = "a"; + builder[1] = "b"; + builder.Clear(); + Assert.Equal(new string[] { null, null }, builder.ExtractToImmutable()); + } + protected override IEnumerable GetEnumerableOf(params T[] contents) { var builder = new ImmutableArray.Builder(contents.Length); diff --git a/src/System.Collections.Immutable/tests/ImmutableArrayTest.cs b/src/System.Collections.Immutable/tests/ImmutableArrayTest.cs index 24828f78b385..8040e9a3b2ae 100644 --- a/src/System.Collections.Immutable/tests/ImmutableArrayTest.cs +++ b/src/System.Collections.Immutable/tests/ImmutableArrayTest.cs @@ -244,6 +244,14 @@ public void CreateFromNullArray() Assert.Equal(0, immutable.Length); } + [Fact] + public void CreateWithCount() + { + Assert.Equal(2, ImmutableArray.CreateBuilderWithCount(2).Count); + Assert.Equal(0, ImmutableArray.CreateBuilderWithCount(0).Count); + Assert.Throws(typeof(ArgumentOutOfRangeException), () => ImmutableArray.CreateBuilderWithCount(-1)); + } + [Fact] public void Covariance() { From 0be9fb3b0c3ab3bab720955d32a68b360a99970b Mon Sep 17 00:00:00 2001 From: Jared Parsons Date: Wed, 3 Dec 2014 13:41:13 -0800 Subject: [PATCH 3/8] Respond to PR feedback --- .../Collections/Immutable/ImmutableArray.cs | 13 ------------- .../Immutable/ImmutableArray`1+Builder.cs | 13 ------------- .../tests/ImmutableArrayBuilderTest.cs | 15 +++++++++++---- .../tests/ImmutableArrayTest.cs | 8 -------- 4 files changed, 11 insertions(+), 38 deletions(-) diff --git a/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray.cs b/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray.cs index ae54882950f5..3faf23899716 100644 --- a/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray.cs +++ b/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray.cs @@ -396,19 +396,6 @@ public static ImmutableArray.Builder CreateBuilder(int initialCapacity) return new ImmutableArray.Builder(initialCapacity); } - /// - /// Initializes a new instance of the class with - /// the capacity and count set to the specified value. - /// - /// The type of elements stored in the array. - /// The count of the underlying initial backing array and builder. - /// A new builder. - [Pure] - public static ImmutableArray.Builder CreateBuilderWithCount(int count) - { - return new ImmutableArray.Builder(capacity: count, count: count); - } - /// /// Enumerates a sequence exactly once and produces an immutable array of its contents. /// diff --git a/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray`1+Builder.cs b/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray`1+Builder.cs index 7a8a9db644d6..dfccfb2d0be0 100644 --- a/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray`1+Builder.cs +++ b/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray`1+Builder.cs @@ -40,19 +40,6 @@ internal Builder(int capacity) _count = 0; } - /// - /// Initializes a new instance of the class. - /// - /// The initial capacity of the internal array. - /// The initial count of the builder. - internal Builder(int capacity, int count) - { - Requires.Range(capacity >= 0, "capacity"); - Requires.Range(count >= 0 && count <= capacity, "count"); - _elements = new T[capacity]; - _count = count; - } - /// /// Initializes a new instance of the class. /// diff --git a/src/System.Collections.Immutable/tests/ImmutableArrayBuilderTest.cs b/src/System.Collections.Immutable/tests/ImmutableArrayBuilderTest.cs index 2ae212f92e66..aecbdc05e5e7 100644 --- a/src/System.Collections.Immutable/tests/ImmutableArrayBuilderTest.cs +++ b/src/System.Collections.Immutable/tests/ImmutableArrayBuilderTest.cs @@ -472,7 +472,7 @@ public void IEnumerator() [Fact] public void ExtractToImmutableNormal() { - var builder = ImmutableArray.CreateBuilderWithCount(2); + var builder = CreateBuilderWithCount(2); Assert.Equal(2, builder.Count); Assert.Equal(2, builder.Capacity); builder[1] = "b"; @@ -486,7 +486,7 @@ public void ExtractToImmutableNormal() [Fact] public void ExtractToImmutableRepeat() { - var builder = ImmutableArray.CreateBuilderWithCount(2); + var builder = CreateBuilderWithCount(2); builder[0] = "a"; builder[1] = "b"; var array1 = builder.ExtractToImmutable(); @@ -510,7 +510,7 @@ public void ExtractToImmutablePartialFill() [Fact] public void ExtractToImmutableThenUse() { - var builder = ImmutableArray.CreateBuilderWithCount(2); + var builder = CreateBuilderWithCount(2); Assert.Equal(2, builder.ExtractToImmutable().Length); Assert.Equal(0, builder.Capacity); builder.Add("a"); @@ -523,13 +523,20 @@ public void ExtractToImmutableThenUse() [Fact] public void ExtractToImmutableAfterClear() { - var builder = ImmutableArray.CreateBuilderWithCount(2); + var builder = CreateBuilderWithCount(2); builder[0] = "a"; builder[1] = "b"; builder.Clear(); Assert.Equal(new string[] { null, null }, builder.ExtractToImmutable()); } + private static ImmutableArray.Builder CreateBuilderWithCount(int count) + { + var builder = ImmutableArray.CreateBuilder(count); + builder.Count = count; + return builder; + } + protected override IEnumerable GetEnumerableOf(params T[] contents) { var builder = new ImmutableArray.Builder(contents.Length); diff --git a/src/System.Collections.Immutable/tests/ImmutableArrayTest.cs b/src/System.Collections.Immutable/tests/ImmutableArrayTest.cs index 8040e9a3b2ae..24828f78b385 100644 --- a/src/System.Collections.Immutable/tests/ImmutableArrayTest.cs +++ b/src/System.Collections.Immutable/tests/ImmutableArrayTest.cs @@ -244,14 +244,6 @@ public void CreateFromNullArray() Assert.Equal(0, immutable.Length); } - [Fact] - public void CreateWithCount() - { - Assert.Equal(2, ImmutableArray.CreateBuilderWithCount(2).Count); - Assert.Equal(0, ImmutableArray.CreateBuilderWithCount(0).Count); - Assert.Throws(typeof(ArgumentOutOfRangeException), () => ImmutableArray.CreateBuilderWithCount(-1)); - } - [Fact] public void Covariance() { From f51ba881e7a402b14938ed4629d2c17f63f28b72 Mon Sep 17 00:00:00 2001 From: Jared Parsons Date: Tue, 6 Jan 2015 09:38:42 -0800 Subject: [PATCH 4/8] Fix up bad rebase merge --- .../System/Collections/Immutable/ImmutableArray`1+Builder.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray`1+Builder.cs b/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray`1+Builder.cs index dfccfb2d0be0..9b8273322dad 100644 --- a/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray`1+Builder.cs +++ b/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray`1+Builder.cs @@ -392,7 +392,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; From e034c931cb1ef0d74b304f6287b185da3c14340a Mon Sep 17 00:00:00 2001 From: Jared Parsons Date: Wed, 7 Jan 2015 14:51:36 -0800 Subject: [PATCH 5/8] Changed name to MoveToImmutable As per the results of the design review this changes the Builder API around extraction of immutable arrays in the following ways: 1. Renamed method to MoveToImmutable 2. Throw when Count != Capacity in that method to avoid potential user errors around extraction. --- .../src/Strings.Designer.cs | 12 ++++++- .../src/Strings.resx | 3 ++ .../Immutable/ImmutableArray`1+Builder.cs | 13 +++++-- .../tests/ImmutableArrayBuilderTest.cs | 36 ++++++++++++------- 4 files changed, 48 insertions(+), 16 deletions(-) diff --git a/src/System.Collections.Immutable/src/Strings.Designer.cs b/src/System.Collections.Immutable/src/Strings.Designer.cs index b0db05c688d5..b2b8787b8e3a 100644 --- a/src/System.Collections.Immutable/src/Strings.Designer.cs +++ b/src/System.Collections.Immutable/src/Strings.Designer.cs @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ // // This code was generated by a tool. -// Runtime Version:4.0.30319.35312 +// Runtime Version:4.0.30319.34014 // // Changes to this file may cause incorrect behavior and will be lost if // the code is regenerated. @@ -9,6 +9,7 @@ //------------------------------------------------------------------------------ namespace System.Collections.Immutable { + using System; using System.Reflection; @@ -87,6 +88,15 @@ internal static string CannotFindOldValue { } } + /// + /// Looks up a localized string similar to MoveToImmutable can only be performed when Count equals Capacity.. + /// + internal static string CapacityMustEqualCountOnMove { + get { + return ResourceManager.GetString("CapacityMustEqualCountOnMove", resourceCulture); + } + } + /// /// Looks up a localized string similar to Collection was modified; enumeration operation may not execute.. /// diff --git a/src/System.Collections.Immutable/src/Strings.resx b/src/System.Collections.Immutable/src/Strings.resx index b3cd27138204..de5e339661bf 100644 --- a/src/System.Collections.Immutable/src/Strings.resx +++ b/src/System.Collections.Immutable/src/Strings.resx @@ -126,6 +126,9 @@ Cannot find the old value + + MoveToImmutable can only be performed when Count equals Capacity. + Collection was modified; enumeration operation may not execute. diff --git a/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray`1+Builder.cs b/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray`1+Builder.cs index 9b8273322dad..159d588750e3 100644 --- a/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray`1+Builder.cs +++ b/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray`1+Builder.cs @@ -149,7 +149,7 @@ bool ICollection.IsReadOnly /// An immutable array. public ImmutableArray ToImmutable() { - if (this.Count == 0) + if (Count == 0) { return Empty; } @@ -159,10 +159,17 @@ public ImmutableArray ToImmutable() /// /// Extracts the internal array as an and replaces it - /// with a zero length array + /// with a zero length array. /// - public ImmutableArray ExtractToImmutable() + /// When doesn't + /// equal . + public ImmutableArray MoveToImmutable() { + if (Capacity != Count) + { + throw new InvalidOperationException(Strings.CapacityMustEqualCountOnMove); + } + var temp = _elements; _elements = ImmutableArray.Empty.array; _count = 0; diff --git a/src/System.Collections.Immutable/tests/ImmutableArrayBuilderTest.cs b/src/System.Collections.Immutable/tests/ImmutableArrayBuilderTest.cs index aecbdc05e5e7..9cfa9083767f 100644 --- a/src/System.Collections.Immutable/tests/ImmutableArrayBuilderTest.cs +++ b/src/System.Collections.Immutable/tests/ImmutableArrayBuilderTest.cs @@ -470,64 +470,76 @@ public void IEnumerator() } [Fact] - public void ExtractToImmutableNormal() + public void MoveToImmutableNormal() { var builder = CreateBuilderWithCount(2); Assert.Equal(2, builder.Count); Assert.Equal(2, builder.Capacity); builder[1] = "b"; builder[0] = "a"; - var array = builder.ExtractToImmutable(); + var array = builder.MoveToImmutable(); Assert.Equal(new[] { "a", "b" }, array); Assert.Equal(0, builder.Count); Assert.Equal(0, builder.Capacity); } [Fact] - public void ExtractToImmutableRepeat() + public void MoveToImmutableRepeat() { var builder = CreateBuilderWithCount(2); builder[0] = "a"; builder[1] = "b"; - var array1 = builder.ExtractToImmutable(); - var array2 = builder.ExtractToImmutable(); + var array1 = builder.MoveToImmutable(); + var array2 = builder.MoveToImmutable(); Assert.Equal(new[] { "a", "b" }, array1); Assert.Equal(0, array2.Length); } [Fact] - public void ExtractToImmutablePartialFill() + public void MoveToImmutablePartialFill() { var builder = ImmutableArray.CreateBuilder(4); builder.Add(42); builder.Add(13); Assert.Equal(4, builder.Capacity); Assert.Equal(2, builder.Count); - var array = builder.ExtractToImmutable(); + Assert.Throws(typeof(InvalidOperationException), () => builder.MoveToImmutable()); + } + + [Fact] + public void MoveToImmutablePartialFillWithCountUpdate() + { + var builder = ImmutableArray.CreateBuilder(4); + builder.Add(42); + builder.Add(13); + Assert.Equal(4, builder.Capacity); + Assert.Equal(2, builder.Count); + builder.Count = builder.Capacity; + var array = builder.MoveToImmutable(); Assert.Equal(new[] { 42, 13, 0, 0 }, array); } [Fact] - public void ExtractToImmutableThenUse() + public void MoveToImmutableThenUse() { var builder = CreateBuilderWithCount(2); - Assert.Equal(2, builder.ExtractToImmutable().Length); + Assert.Equal(2, builder.MoveToImmutable().Length); Assert.Equal(0, builder.Capacity); builder.Add("a"); builder.Add("b"); Assert.Equal(2, builder.Count); Assert.True(builder.Capacity >= 2); - Assert.Equal(new[] { "a", "b" }, builder.ExtractToImmutable()); + Assert.Equal(new[] { "a", "b" }, builder.MoveToImmutable()); } [Fact] - public void ExtractToImmutableAfterClear() + public void MoveToImmutableAfterClear() { var builder = CreateBuilderWithCount(2); builder[0] = "a"; builder[1] = "b"; builder.Clear(); - Assert.Equal(new string[] { null, null }, builder.ExtractToImmutable()); + Assert.Throws(typeof(InvalidOperationException), () => builder.MoveToImmutable()); } private static ImmutableArray.Builder CreateBuilderWithCount(int count) From c743cfb41f17b2d07d56449de207198de08f244c Mon Sep 17 00:00:00 2001 From: Jared Parsons Date: Wed, 7 Jan 2015 15:21:18 -0800 Subject: [PATCH 6/8] Make Builder more like List As discussed making two minor changes to make Builder line up closer to List - Capacity now has a get and set. - Make EnsureCapacity a private method. It is superfluous in the public API with the addition of a set accessor on Capacity. --- .../src/Strings.Designer.cs | 9 ++++ .../src/Strings.resx | 3 ++ .../Immutable/ImmutableArray`1+Builder.cs | 30 ++++++++++- .../tests/ImmutableArrayBuilderTest.cs | 52 +++++++++++++++++++ 4 files changed, 92 insertions(+), 2 deletions(-) diff --git a/src/System.Collections.Immutable/src/Strings.Designer.cs b/src/System.Collections.Immutable/src/Strings.Designer.cs index b2b8787b8e3a..18031e9d16a6 100644 --- a/src/System.Collections.Immutable/src/Strings.Designer.cs +++ b/src/System.Collections.Immutable/src/Strings.Designer.cs @@ -88,6 +88,15 @@ internal static string CannotFindOldValue { } } + /// + /// Looks up a localized string similar to Capacity was less than the current Count of elements.. + /// + internal static string CapacityMustBeGreaterThanOrEqualToCount { + get { + return ResourceManager.GetString("CapacityMustBeGreaterThanOrEqualToCount", resourceCulture); + } + } + /// /// Looks up a localized string similar to MoveToImmutable can only be performed when Count equals Capacity.. /// diff --git a/src/System.Collections.Immutable/src/Strings.resx b/src/System.Collections.Immutable/src/Strings.resx index de5e339661bf..540ba24c7235 100644 --- a/src/System.Collections.Immutable/src/Strings.resx +++ b/src/System.Collections.Immutable/src/Strings.resx @@ -126,6 +126,9 @@ Cannot find the old value + + Capacity was less than the current Count of elements. + MoveToImmutable can only be performed when Count equals Capacity. diff --git a/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray`1+Builder.cs b/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray`1+Builder.cs index 159d588750e3..1eca90b96a19 100644 --- a/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray`1+Builder.cs +++ b/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray`1+Builder.cs @@ -49,11 +49,37 @@ internal Builder() } /// - /// Get the length of the internal 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. /// public int Capacity { get { return _elements.Length; } + set + { + if (value < _count) + { + throw new ArgumentException(Strings.CapacityMustBeGreaterThanOrEqualToCount, paramName: "value"); + } + + if (value != _elements.Length) + { + if (value > 0) + { + var temp = new T[value]; + if (_count > 0) + { + Array.Copy(_elements, 0, temp, 0, _count); + } + + _elements = temp; + } + else + { + _elements = ImmutableArray.Empty.array; + } + } + } } /// @@ -421,7 +447,7 @@ public void CopyTo(T[] array, int index) /// Resizes the array to accommodate the specified capacity requirement. /// /// The required capacity. - public void EnsureCapacity(int capacity) + private void EnsureCapacity(int capacity) { if (_elements.Length < capacity) { diff --git a/src/System.Collections.Immutable/tests/ImmutableArrayBuilderTest.cs b/src/System.Collections.Immutable/tests/ImmutableArrayBuilderTest.cs index 9cfa9083767f..42dd675b5e38 100644 --- a/src/System.Collections.Immutable/tests/ImmutableArrayBuilderTest.cs +++ b/src/System.Collections.Immutable/tests/ImmutableArrayBuilderTest.cs @@ -542,6 +542,58 @@ public void MoveToImmutableAfterClear() Assert.Throws(typeof(InvalidOperationException), () => builder.MoveToImmutable()); } + [Fact] + public void CapacitySetToZero() + { + var builder = ImmutableArray.CreateBuilder(initialCapacity: 10); + builder.Capacity = 0; + Assert.Equal(0, builder.Capacity); + Assert.Equal(new int[] { }, builder.ToArray()); + } + + [Fact] + public void CapacitySetToLessThanCount() + { + var builder = ImmutableArray.CreateBuilder(initialCapacity: 10); + builder.Add(1); + builder.Add(1); + Assert.Throws(typeof(ArgumentException), () => builder.Capacity = 1); + } + + [Fact] + public void CapacitySetToCount() + { + var builder = ImmutableArray.CreateBuilder(initialCapacity: 10); + builder.Add(1); + builder.Add(2); + builder.Capacity = builder.Count; + Assert.Equal(2, builder.Capacity); + Assert.Equal(new[] { 1, 2 }, builder.ToArray()); + } + + [Fact] + public void CapacitySetToCapacity() + { + var builder = ImmutableArray.CreateBuilder(initialCapacity: 10); + builder.Add(1); + builder.Add(2); + builder.Capacity = builder.Capacity; + Assert.Equal(10, builder.Capacity); + Assert.Equal(new[] { 1, 2 }, builder.ToArray()); + } + + [Fact] + public void CapacitySetToBiggerCapacity() + { + var builder = ImmutableArray.CreateBuilder(initialCapacity: 10); + builder.Add(1); + builder.Add(2); + builder.Capacity = 20; + Assert.Equal(20, builder.Capacity); + Assert.Equal(2, builder.Count); + Assert.Equal(new[] { 1, 2 }, builder.ToArray()); + } + private static ImmutableArray.Builder CreateBuilderWithCount(int count) { var builder = ImmutableArray.CreateBuilder(count); From dd2b6ea8ad04989a6e97f3eef23ccd412c7077eb Mon Sep 17 00:00:00 2001 From: Jared Parsons Date: Thu, 8 Jan 2015 17:36:12 -0800 Subject: [PATCH 7/8] Added some more tests --- .../tests/ImmutableArrayBuilderTest.cs | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/src/System.Collections.Immutable/tests/ImmutableArrayBuilderTest.cs b/src/System.Collections.Immutable/tests/ImmutableArrayBuilderTest.cs index 42dd675b5e38..54034928e06f 100644 --- a/src/System.Collections.Immutable/tests/ImmutableArrayBuilderTest.cs +++ b/src/System.Collections.Immutable/tests/ImmutableArrayBuilderTest.cs @@ -542,6 +542,53 @@ public void MoveToImmutableAfterClear() Assert.Throws(typeof(InvalidOperationException), () => builder.MoveToImmutable()); } + [Fact] + public void MoveToImmutableAddToCapacity() + { + var builder = ImmutableArray.CreateBuilder(initialCapacity: 3); + for (int i = 0; i < builder.Capacity; i++) + { + builder.Add(i); + } + + Assert.Equal(new[] { 0, 1, 2 }, builder.MoveToImmutable()); + } + + [Fact] + public void MoveToImmutableInsertToCapacity() + { + var builder = ImmutableArray.CreateBuilder(initialCapacity: 3); + for (int i = 0; i < builder.Capacity; i++) + { + builder.Insert(i, i); + } + + Assert.Equal(new[] { 0, 1, 2 }, builder.MoveToImmutable()); + } + + [Fact] + public void MoveToImmutableAddRangeToCapcity() + { + var array = new[] { 1, 2, 3, 4, 5 }; + var builder = ImmutableArray.CreateBuilder(initialCapacity: array.Length); + builder.AddRange(array); + Assert.Equal(array, builder.MoveToImmutable()); + } + + [Fact] + public void MoveToImmutableAddRemoveAddToCapacity() + { + var builder = ImmutableArray.CreateBuilder(initialCapacity: 3); + for (int i = 0; i < builder.Capacity; i++) + { + builder.Add(i); + builder.RemoveAt(i); + builder.Add(i); + } + + Assert.Equal(new[] { 0, 1, 2 }, builder.MoveToImmutable()); + } + [Fact] public void CapacitySetToZero() { From d4f998efa34b67e4ad65e770aea553e8b82ad012 Mon Sep 17 00:00:00 2001 From: Jared Parsons Date: Fri, 9 Jan 2015 06:57:16 -0800 Subject: [PATCH 8/8] Respond to PR feedback --- .../System/Collections/Immutable/ImmutableArray`1+Builder.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray`1+Builder.cs b/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray`1+Builder.cs index 1eca90b96a19..d2e99ee4daf6 100644 --- a/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray`1+Builder.cs +++ b/src/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray`1+Builder.cs @@ -196,7 +196,7 @@ public ImmutableArray MoveToImmutable() throw new InvalidOperationException(Strings.CapacityMustEqualCountOnMove); } - var temp = _elements; + T[] temp = _elements; _elements = ImmutableArray.Empty.array; _count = 0; return new ImmutableArray(temp);