Improvement on Immutable Lists - #10
Conversation
2e2e8e8 to
0bfaa2a
Compare
| { | ||
| this.data = data; | ||
| _data = new T[data.Length]; | ||
| Array.Copy(data, _data, data.Length); |
There was a problem hiding this comment.
Does copying all elements each time is not the perf issue we try to avoid? At least I think we should add a private ctor which allow to bypass this.
| public int IndexOf(T item, int index, int count, IEqualityComparer<T> equalityComparer) | ||
| { | ||
| var comparer = equalityComparer ?? EqualityComparer<T>.Default; | ||
| for (var i = index; i < index + count; i++) |
There was a problem hiding this comment.
Possible OutOfRange. How does behave the ImmutableList from ImmutableCollections ?
There was a problem hiding this comment.
Just checked: yes, same behavior.
| public int LastIndexOf(T item, int index, int count, IEqualityComparer<T> equalityComparer) | ||
| { | ||
| var comparer = equalityComparer ?? EqualityComparer<T>.Default; | ||
| for (var i = index + count - 1; i >= index; i--) |
There was a problem hiding this comment.
Same thing here, possible OutOfRange
There was a problem hiding this comment.
Just checked: yes, same behavior.
| var newData = new T[_data.Length + itemsToAdd.Length]; | ||
| Array.Copy(_data, newData, _data.Length); | ||
| Array.Copy(itemsToAdd, 0, newData, _data.Length, itemsToAdd.Length); | ||
| return new ImmutableList<T>(newData); |
There was a problem hiding this comment.
Here an exemple of why we should have a constructor which does not create a new copy of the array
| public IImmutableList<T> RemoveAll(Predicate<T> match) | ||
| { | ||
| var newData = _data.Where(x => !match(x)).ToArray(); | ||
| return new ImmutableList<T>(newData); |
There was a problem hiding this comment.
If there is not matches, we should return this
|
|
||
| public IImmutableList<T> RemoveAll(Predicate<T> match) | ||
| { | ||
| var newData = _data.Where(x => !match(x)).ToArray(); |
There was a problem hiding this comment.
Is there a reason why you used Linq here but nowhere else in the class?
| } | ||
|
|
||
|
|
||
|
|
|
|
||
| public IImmutableList<T> SetItem(int index, T value) | ||
| { | ||
| if (Equals(_data[index], value)) |
There was a problem hiding this comment.
I think here you should use ReferenceEquals
There was a problem hiding this comment.
No we need to invoke equality members there.
|
|
||
| public IImmutableList<T> Replace(T oldValue, T newValue, IEqualityComparer<T> equalityComparer) | ||
| { | ||
| if (Equals(oldValue, newValue)) |
| /// Creates an option for a given value. | ||
| /// </summary> | ||
| public static Some<T> Some<T>(T value) => new Some<T>(value); | ||
| public static Option<T> Some<T>(T value) => new Some<T>(value); |
There was a problem hiding this comment.
That's a binary breaking change... why is it needed ?
There was a problem hiding this comment.
It has been delayed in the past because of this reason. Was waiting for the right moment to push it. We can put it back on backburner if you want.
8867da9 to
1f98923
Compare
| get { return _empty; } | ||
| /// <param name="data">An array as source</param> | ||
| /// <param name="copyData">If the array should be copied</param> | ||
| public ImmutableList(T[] data, bool copyData = false) |
There was a problem hiding this comment.
Do we want to let this public ? If so, does the copyData should not be set to true by default ?
There was a problem hiding this comment.
copyData is false by default because of existing code using it right now - as it's the previous behavior.
@jeromelaban what do you think?
1f98923 to
836df78
Compare
3 participants
Uno.Core's version of
ImmutableList<T>implementsIImmutableList<T>from Microsoft's Immutable Collections.