[10?] RFR: 8193128: Reduce number of implementation classes returned by List/Set/Map.of()

[Stuart Marks]

Finally catching up with this thread....

Yes, there should be some additional test cases added. When I added the 
immutable collection classes in JDK 9, I did modify MOAT.java so that its test 
cases would apply to the new implementations.

A few more cases could be added that apply not only to the immutable lists but 
also to lists in general.

I think the bugs mentioned here are with indexOf() and lastIndexOf(), with the 
latter accidentally being a copy of the former. Later in the thread John pointed 
out an off-by-one error with lastIndexOf(), so edge cases should be added as 
well. These would apply to all lists, not just the immutable ones.

There is also the issue mentioned in

     JDK-8191418 List.of().indexOf(null) doesn't throw NullPointerException

Tests should be added for that and related methods. Since many collections 
permit null, and I suspect some that disallow null might permit indexOf(null) 
and related, it's not clear to me these belong in MOAT.java.

But if List.of().indexOf(null) were to throw NPE, I'd expect 
List.of().subList(...).indexOf(null) also to throw NPE.

s'marks


On 12/8/17 9:44 AM, Martin Buchholz wrote:
> 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.redestad at oracle.com 
> <mailto:claes.redestad at oracle.com>> wrote:
> 
>     Hi,
> 
>     On 2017-12-08 07:54, Andrej Golovnin wrote:
> 
>         Hi Claes,
> 
>             http://cr.openjdk.java.net/~redestad/8193128/open.02/
>             <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/
>     <http://cr.openjdk.java.net/~redestad/8193128/open.03/>
> 
>     Thanks!
> 
>     /Claes
> 
>     [1] https://bugs.openjdk.java.net/browse/JDK-8191442
>     <https://bugs.openjdk.java.net/browse/JDK-8191442>
> 
>