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
