Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add method wrapCopy to Primitive Lists to mirror the functionality in FastList #742

Closed
mustangxu opened this issue Aug 13, 2019 · 2 comments · Fixed by #831
Closed

Add method wrapCopy to Primitive Lists to mirror the functionality in FastList #742

mustangxu opened this issue Aug 13, 2019 · 2 comments · Fixed by #831
Assignees

Comments

@mustangxu
Copy link

Let me take MutableIntList as an example.

int[] array = { 1, 2, 3, 4 };
MutableIntList list = IntLists.mutable.of(array);

In the source code of IntArrayList, the constructor takes an initial array as parameter and holds the reference to it in this.items:

public IntArrayList(int... array)
{
this.size = array.length;
this.items = array;
}

When invoke removeAtIndex(), the item will be removed from both the list and the initial array (this behavior is a disaster and should be documented clearly!):

list.removeAtIndex(0);
System.out.println(Arrays.toString(array)); // [2,3,4,0]

On the other hand, if invoke add() before removeAtIndex(), due to ensureCapacityForAdd(), this.items will reference to a new larger array, and removeAtIndex() will no longer affect the initial array:

list.add(5);
list.removeAtIndex(0);
System.out.println(Arrays.toString(array)); // [1,2,3,4]

So for Mutable Primitive List, pls consider

  • make a array copy in the constructor instead of holding the reference
  • if removing items from the initial array is by design, pls make the behavior clear in the document, and, add another version of removeAtIndex() which affects on the list only
@mohrezaei
Copy link
Member

Eclipse collections is used by applications that are very sensitive to garbage. Adding extra copies is not necessary most of the time and therefore we leave that to the application. The behavior is documented on the actual IntArrayList.newListWith method, but it's not documenteed on the factory method, or various methods that delegate to it. It would be good to copy the warning to those methods.

    /**
     * Creates a new list using the passed {@code elements} argument as the backing store.
     * <p>
     * !!! WARNING: This method uses the passed in array, so can be very unsafe if the original
     * array is held onto anywhere else. !!!
     */

@donraab
Copy link
Contributor

donraab commented Apr 1, 2020

We can add a method named wrapCopy which would mirror what we do on FastList. I will edit the title of this issue to reflect that.

@donraab donraab changed the title Mutable Primitive List should not hold the reference of the initial array Add method wrapCopy to Primitive Lists to mirror the functionality in FastList Apr 1, 2020
@donraab donraab self-assigned this Apr 2, 2020
donraab added a commit to donraab/eclipse-collections that referenced this issue Apr 2, 2020
…tList. Fixes eclipse#742.

Signed-off-by: Donald Raab <Donald.Raab@bnymellon.com>
donraab added a commit to donraab/eclipse-collections that referenced this issue Apr 2, 2020
…tList. Fixes eclipse#742.

Signed-off-by: Donald Raab <Donald.Raab@bnymellon.com>
donraab added a commit to donraab/eclipse-collections that referenced this issue Apr 2, 2020
…tList. Fixes eclipse#742.

Signed-off-by: Donald Raab <Donald.Raab@bnymellon.com>
donraab added a commit to donraab/eclipse-collections that referenced this issue Apr 2, 2020
…tList. Fixes eclipse#742.

Signed-off-by: Donald Raab <Donald.Raab@bnymellon.com>
aboullaite pushed a commit to aboullaite/eclipse-collections that referenced this issue Apr 5, 2020
…tList. Fixes eclipse#742.

Signed-off-by: Donald Raab <Donald.Raab@bnymellon.com>
aboullaite pushed a commit to aboullaite/eclipse-collections that referenced this issue Apr 11, 2020
…tList. Fixes eclipse#742.

Signed-off-by: Donald Raab <Donald.Raab@bnymellon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants