[10?] RFR: 8193128: Reduce number of implementation classes returned by List/Set/Map.of()
stuart.marks at oracle.com
Fri Dec 22 01:04:07 UTC 2017
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.
On 12/8/17 9:44 AM, Martin Buchholz wrote:
> Should there be changes to general purpose collection testing classes like
> 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:
> On 2017-12-08 07:54, Andrej Golovnin wrote:
> Hi Claes,
> 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, 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:
>  https://bugs.openjdk.java.net/browse/JDK-8191442
More information about the core-libs-dev