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

[Claes Redestad]

Hi Stuart,

On 2018-03-22 01:51, Stuart Marks wrote:
> Hi Claes,
>
> I'm finally finally getting back to this. I reviewed the current state 
> of the patch located here:
>
>     http://cr.openjdk.java.net/~redestad/8193128/open.06/
>
> I think this is mostly fine as it is. 

thanks!

> There are some things about it that could be adjusted, but none of 
> them stand in the way of it going in. If you want to take care of any 
> of these items before pushing, that'd be great, otherwise they can be 
> handled separately later.
>
> * AbstractImmutableCollection and AbstractImmutableSet are in the 
> "List Implementations" section of the file. Seems like AIC ought to be 
> moved to the top, since it's common to List and Set, and AIS ought to 
> be moved to the "Set Implementations" section. This is just location 
> in the file, not nesting level.

Fixed.

>
> * The SubList constructors that are overloaded based on the type of 
> the 1st arg (List vs SubList) seems subtle and error-prone. I misread 
> the code the first time I saw it. Seems like it would be preferable to 
> have well-named static factory methods, each calling a policy-free 
> (private) constructor.

Introduced SubList.fromList and SubList.fromSubList to make this clearer.

>
> * Should SubList.size be final? Should any of the SubList fields be 
> @Stable?

Good point, this aligns the implementation more with the other immutable 
classes here.

>
> * Does the SubList class need to nested within AbstractImmutableList? 
> Note that it also extends AIL, so I don't think nesting gives it 
> access to anything that it doesn't already have. Plus it includes an 
> anonymous inner class based on ListIterator, which is a fourth level 
> of nested classes. It'd be good to flatten this out a bit if possible.

Trivially flattened, done.

>
> * The instance returned by SubList.iterator() is also a ListIterator. 
> Hmmm. But I'm also wondering if SubList's iterators can be shared 
> somehow with AIL's Itr and ListItr.

With an immutable backing store a ListIterator doesn't add any 
capabilities over Iterator that can be considered unsafe or unsavory - 
adds a few methods, no extra state - so it should be performance 
neutral. I actually think the Itr/ListItr also ought to be consolidated 
into one class implementing ListIterator, now that you made me look... 
I'm open to the possibility I'm wrong on this one, though. :-)

Turning ListItr into a static class whose constructors take the List 
being iterated over is performance neutral here as we only rely on 
List::get(i), which enable sharing implementation with SubList.

>
> * Several indexOf tests are added to ListFactories.java. Are these 
> redundant with the indexOf tests that were added to MOAT?

Right, I added the generic indexOf tests to MOAT later, and it does seem 
the ListFactories.indexOf test are now redundant. Removed.

Webrev: http://cr.openjdk.java.net/~redestad/8193128/open.07/
Incremental: http://cr.openjdk.java.net/~redestad/8193128/open.06_to_07/

Hope these incremental changes look OK.

/Claes