ArrayList.removeAll()/retainAll()

[Benedict Elliott Smith]

Hi Mike,

Thanks very much for the careful explanation, it's appreciated. It does
look trivial to fix, so I'll see about raising a ticket over there.


On 4 February 2014 17:35, Mike Duigou <mike.duigou at oracle.com> wrote:

> The condition that is causing the problem is not a collection containing
> null, which is allowed, but that the provided collection "c" is itself null.
>
> The problem was is a consequence of the following implementation:
>
> Iterator<E> iterator = iterator();
>
> while(iterator.hasNext()) {
>  if(c.contains(iterator.next()) {
>     iterator.remove();
>  }
> }
>
> This implementation had an inconsistent behaviour. If a collection
> contains elements then the first iteration of the while loops will cause a
> NullPointerException if "c" is null. If, however, the collection is empty
> then c is never referenced and no NPE is thrown. The change in java 8 which
> adds an explicit Objects.requireNonNull(c) ensures the behaviour is
> consistent whether the collection is empty or not. Passing null as the
> parameter for removeAll and retainAll has never been valid. What's changed
> is that it's now consistently checked.
>
> It's unfortunate that this particular fix tickled a bug in Antlr 3. It
> appears that it's simply a bug that Antlr doesn't check the result of it's
> method which returns null. (
> https://github.com/antlr/antlr3/blob/master/tool/src/main/java/org/antlr/tool/CompositeGrammar.java#L226)
> The fix would seem to be relatively straightforward to either have
> getDirectDelegates() return Collections.<Grammar>emptyList() or add a check
> for null. My personal preference would be for the former as it allows for
> uniform handling of the result wherever the method is called without having
> to remember that the result might be null.
>
> Mike
>
> On Feb 4 2014, at 09:00 , Benedict Elliott Smith <
> belliottsmith at datastax.com> wrote:
>
> > Hi,
> >
> > I notice this (or a related issue) has been mentioned
> > before<
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/017663.html
> >on
> > this list, but I'm not convinced the correct resolution was reached.
> > We
> > are seeing this problem thrown by antlr, but rather than a bug in antlr,
> as
> > surmised on the previous exchange, it looks to me that ArrayList is
> > imposing a new constraint that is neither declared by itself nor
> > Collection, and is unnecessary. ArrayList happily supports null elements,
> > so requiring that the provided collection has no null elements is surely
> a
> > bug?
> >
> > I've pasted the two declarations below for ease of reference. Neither
> > javadocs describe the constraint that is imposed.
> >
> > ArrayList:
> >    /**
> >     * Removes from this list all of its elements that are contained in
> the
> >     * specified collection.
> >     *
> >     * @param c collection containing elements to be removed from this
> list
> >     * @return {@code true} if this list changed as a result of the call
> >     * @throws ClassCastException if the class of an element of this list
> >     *         is incompatible with the specified collection
> >     * (<a href="Collection.html#optional-restrictions">optional</a>)
> >     * @throws NullPointerException if this list contains a null element
> > and the
> >     *         specified collection does not permit null elements
> >     * (<a href="Collection.html#optional-restrictions">optional</a>),
> >     *         or if the specified collection is null
> >     * @see Collection#contains(Object)
> >     */
> >    public boolean removeAll(Collection<?> c) {
> >        Objects.requireNonNull(c);
> >        return batchRemove(c, false);
> >    }
> >
> > Collection:
> >    /**
> >     * Removes all of this collection's elements that are also contained
> in
> > the
> >     * specified collection (optional operation).  After this call
> returns,
> >     * this collection will contain no elements in common with the
> specified
> >     * collection.
> >     *
> >     * @param c collection containing elements to be removed from this
> > collection
> >     * @return <tt>true</tt> if this collection changed as a result of the
> >     *         call
> >     * @throws UnsupportedOperationException if the <tt>removeAll</tt>
> > method
> >     *         is not supported by this collection
> >     * @throws ClassCastException if the types of one or more elements
> >     *         in this collection are incompatible with the specified
> >     *         collection
> >     *         (<a href="#optional-restrictions">optional</a>)
> >     * @throws NullPointerException if this collection contains one or
> more
> >     *         null elements and the specified collection does not support
> >     *         null elements
> >     *         (<a href="#optional-restrictions">optional</a>),
> >     *         or if the specified collection is null
> >     * @see #remove(Object)
> >     * @see #contains(Object)
> >     */
> >    boolean removeAll(Collection<?> c);
>
>