RFR 8024709 : TreeMap.DescendingKeyIterator 'remove' confuses iterator position

[Paul Sandoz]

On Oct 4, 2013, at 7:28 AM, Alan Bateman <Alan.Bateman at oracle.com> wrote:

> On 03/10/2013 16:31, Brent Christian wrote:
>> Please review my fix for 8024709 : "TreeMap.DescendingKeyIterator 'remove' confuses iterator position"
>> 
>> There are two possible code paths for performing a "descending" iteration over the elements in a TreeMep, depending on how the iteration is setup.
>> 
>> For instance,
>>  treemap.descendingKeySet().iterator();
>> will use a
>>  TreeMap.NavigableSubMap.DescendingSubMapIterator
>> This code correctly handles Iterator.remove().
>> 
>> On the other hand,
>>  treemap.navigableKeySet().descendingIterator();
>> will use a
>>  TreeMap.DescendingKeyIterator.
>> This code does not correctly handle remove(), and results in a "confused" iterator.
>> 
>> 
>> TreeMap.DescendingKeyIterator should override remove() with code
>> similar to that in TreeMap.NavigableSubMap.SubMapIterator.removeDescending().
>> 
>> 
>> Bug report:
>> https://bugs.openjdk.java.net/browse/JDK-8024709
>> 
>> Webrev:
>> http://cr.openjdk.java.net/~bchristi/8024709/webrev.00/
> The remove looks right to me and next() should return the predecessor that is already set.
> 

Looks ok to me too.


> The test looks okay (although you might want to align the parameters to checkDescendingIteratorRemove), I just wonder if you would be worth generating additional cases to exercise this code a bit more.
> 

One could write a general test without having to pass in expicit values and if so i suspect it is better to pass in the ascending collection and descending iterator (since the toArray could be implemented using an iterator):

<T> void checkTraverse2Remove1Traverse1(Collection<T> aC, Iterator<T> dIt) {
  T[] values = (T) aC.toArray();
  int n = values.length -1;

  equalNext(dIt, values[n--]);
  equalNext(dIt, values[n--]);
  it.remove();
  equalNext(dIt, values[n]);
}

Might also be useful to have a test that removes the last traversed element.

Paul.