mkodekar / guava-libraries

Automatically exported from code.google.com/p/guava-libraries
Apache License 2.0
0 stars 0 forks source link

Allow AbstractIterator subclasses to implement remove() #1797

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
AbstractIterator is very handy, however, it does not support subclasses that 
implement remove() because it extends UnmodifiableIterator which has a final 
remove() method.

This could be fixed in a fully backward-compatible way as follows:

1. Don't extend UnmodifiableIterator (just extend Object instead).

2. Add these new methods:

    @Override
    public void remove() {
        throw UnsupportedOperationException();
    }

    /**
     * Set element to be removed on the next invocation to remove().
     */
    protected void setRemoveElement(E element) {
    }

3. Update the next() implementation to invoke this.setRemoveElement() just 
prior to returning the next element.

With these changes, a subclass that wanted to support remove() could override 
setRemoveElement() to record the next removal candidate, and then override 
remove() to remove that candidate if invoked.

Existing subclasses of AbstractIterable would continue to function as they 
currently do.

Original issue reported on code.google.com by archie.c...@gmail.com on 5 Jul 2014 at 11:17

GoogleCodeExporter commented 9 years ago
It seems to me that this complicates AbstractIterator significantly for a 
feature for which there's already a relatively easy workaround (just 
implementing Iterator yourself).

It's also not clear to me that your proposed solution would even be an 
effective way of implementing remove(). Having a reference to the element you 
want to remove on the next remove() call doesn't necessarily help you. Just for 
example, consider an AbstractIterator that wraps another Iterator. After 
calling hasNext() on your AbstractIterator, it'll no longer be possible to 
remove() the element because next() has already been called on the underlying 
Iterator. There's also issues with wrapping a Collection that can contain the 
same element multiple times, such as a List.

Finally, AbstractIterator is not @Beta and I'm not sure we could safely add a 
protected method to it even if we wanted to.

Original comment by cgdecker@google.com on 7 Jul 2014 at 4:55

GoogleCodeExporter commented 9 years ago
Indeed, we tried to make an AbstractIteratorWithRemove class once, and we 
pretty much never found a user using it correctly. We had to back it out.

Original comment by kevinb@google.com on 7 Jul 2014 at 5:57

GoogleCodeExporter commented 9 years ago
See also https://code.google.com/p/google-collections/issues/detail?id=224

AbstractIterator.remove() seems to work best with concurrent collections. 
That's a small enough niche that we think we're best off not trying to handle 
it.

Original comment by cpov...@google.com on 17 Jul 2014 at 12:03

GoogleCodeExporter commented 9 years ago
This issue has been migrated to GitHub.

It can be found at https://github.com/google/guava/issues/<issue id>

Original comment by cgdecker@google.com on 1 Nov 2014 at 4:08

GoogleCodeExporter commented 9 years ago

Original comment by cgdecker@google.com on 3 Nov 2014 at 9:07