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

[Stuart Marks]

Great, thanks for doing those cleanups!

They're small, but I think they make a big difference in the comprehensibility 
of the code, because of all the little classes in this file.

Looks like you went ahead and consolidated everything into ListItr, which is 
used by the iterator() and listIterator() methods of both AIL and SubList. Seems 
reasonable. The mutator methods are all blocked, so this should be fine.

One small issue is with this comment:

  295          * Constructs a sublist of an arbitrary AbstractList, which is
  296          * not a SubList itself.

This supports sublists of an arbitrary AbstractImmutableList, not AbstractList.

Other than this, I think you're good to go!

Thanks,

s'marks


On 3/22/18 8:42 AM, Claes Redestad wrote:
> 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