On Oct 4, 2013, at 7:28 AM, Alan Bateman <alan.bate...@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.
