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
