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. 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.
* 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.
* Should SubList.size be final? Should any of the SubList fields be @Stable?
* 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.
* 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.
* Several indexOf tests are added to ListFactories.java. Are these redundant
with the indexOf tests that were added to MOAT?
Let me know what, if any, you fix up before pushing, and I'll track the rest.
Thanks,
s'marks
