Should there be changes to general purpose collection testing classes like test/jdk/java/util/concurrent/tck/CollectionTest.java test/jdk/java/util/Collection/MOAT.java that would have caught this bug? (although those are mostly designed for mutable collections, but I think some immutable collections (nCopies?) get tested somewhere.
On Fri, Dec 8, 2017 at 7:13 AM, Claes Redestad <claes.redes...@oracle.com> wrote: > Hi, > > On 2017-12-08 07:54, Andrej Golovnin wrote: > >> Hi Claes, >> >> http://cr.openjdk.java.net/~redestad/8193128/open.02/ >>> >> I think you should provide specialised implementations of the >> #indexOf() and #lastIndexOf() methods in the classes List12 and ListN. >> Using an iterator in List12 is an overkill. But even in ListN using a >> simple for loop would be much better. >> > > it's somewhat ironic that I started looking at this *because* > indexOf was slow due use of iterators[1], but then never got > around to specialize them in this patch. > > In any case please take look at >> the implementation of the #lastIndexOf() method in the >> AbstractImmutableList class. It looks like a copy of >> AbstractImmutableList#indexOf() and this is wrong. >> > > Nice catch! Quite the embarrassing copy-paste that... > > - Specialized the index/lastIndexOf methods for List12, ListN > - Fixed implementation of lastIndexOf in AbstractImmutableList. > - As AbstractImmutableList.indexOf/lastIndexOf are now only used > by AbstractImmutableList.SubList, I moved them there with some > commentary since it's not clear JDK-8191418 should add null > checkson the input for SubList or not. > - Added sanity tests of indexOf/lastIndexOf that touches all > the new implementations: > > http://cr.openjdk.java.net/~redestad/8193128/open.03/ > > Thanks! > > /Claes > > [1] https://bugs.openjdk.java.net/browse/JDK-8191442 >
