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.redes...@oracle.com
<mailto: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/
<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>
