potential bug using removeAll() called by a Collection on itself

Question

During a code review using Sonar , the following code has been detected as a bad one:

ArrayList<String> ops = new ArrayList<String>();
ops.add("test");
ops.removeAll(ops);

Sonar is complaining about the removeAll on called by the collection on itself.

I agree that it's ugly but can this introduce bugs?

NB: This is not my code , I'm reviewing it .

Answer 1

Actually, it depends on the version of jdk you are using. In jdk6, most of the collection class inherit the removeAll method from the AbstractCollection class:

public boolean removeAll(Collection<?> c) {
    boolean modified = false;
    Iterator<?> e = iterator();
    while (e.hasNext()) {
        if (c.contains(e.next())) {
        e.remove();
        modified = true;
        }
   }
   return modified;
}

So, in some scenario, this will cause a CME(ConcurrentModificationException), for example:

ArrayList<Integer> it = new ArrayList<Integer>();
it.add(1);
it.add(2);
it.add(3);
List<Integer> sub = it.subList(0,5);
it.removeAll(sub);

however, in jdk8, most collections has its own removeAll implementation, such as ArrayList. The ArrayList class has its own removeAll method,which call a private method batchRemove:

private boolean batchRemove(Collection<?> c, boolean complement) {
        final Object[] elementData = this.elementData;
        int r = 0, w = 0;
        boolean modified = false;
        try {
            for (; r < size; r++)
                if (c.contains(elementData[r]) == complement)
                    elementData[w++] = elementData[r];
        } finally {
            // Preserve behavioral compatibility with AbstractCollection,
            // even if c.contains() throws.
            if (r != size) {
                System.arraycopy(elementData, r,
                                 elementData, w,
                                 size - r);
                w += size - r;
            }
            if (w != size) {
                // clear to let GC do its work
                for (int i = w; i < size; i++)
                    elementData[i] = null;
                modCount += size - w;
                size = w;
                modified = true;
            }
        }
        return modified;
    }

This using for loop iterate over the underlying array, and changing the variable modCount only once after the end of this method, during the iteration of the sublist( invoking by c.contains(elementData[r])), the modCount is not changing, so no CME will be thrown.