Hi Rémi,
On 2017-12-06 23:57, Remi Forax wrote:
Hi Claes,
- both constructors of SubList should be package private,
deal!
- in listIterator, i can be declared outside of the ListIterator as a local
variable that would be captured by the anonymous class,
so index is not used inside the anonymous class. Also you can use the
diamond syntax for anonymous class now.
public ListIterator<E> listIterator(int index) {
rangeCheck(index);
ListIterator<E> i = root.listIterator(offset + index);
return new ListIterator<>() {
...
Sure.
- you can move "new IndexOutOfBoundsException" out of rangeCheck into
outOfBoundsMsg, so the bytecode size of rangeCheck() will be smaller.
I had already done so for the outer class, and realized I had forgotten
to clean this part up:
SubList is now final, inherits from AbstractImmutableList instead of
AbstractList and reuses more of the shared code.
I also moved 'implements Serializable' from AbstractImmutableList to
List12 and ListN respectively to not
change the behavior that List.of(0,1) is serializable while
List.of(0,1).subList(0,1) isn't.
- in Itr, please declare the constructor after the declaration of the fields,
and assigning the cursor to zero is useless.
Done.
- in equals, the code is weirdly asymmetric because e1 is typed as an Iterator<E>,
declaring it as an Iterator<?> will make the code more symmetric.
I also realized I had missed an opportunity to take advantage of the
fact that elements returned from e1 are guaranteed to
be non-null. Might as well.
- in List12, you have a lot of useless @SuppressWarnings("unchecked") ??
in size(), get(), contains(), hashCode(), writeReplace().
- in ListN, again some useless @SuppressWarnings("unchecked") ??
in size(), get(), contains(), hashCode(), writeReplace()
I don't know how these snuck in, but my IDE was pleasantly hiding these
away. I think I cleaned out all the useless ones.
- the constructor of MapN(K,V) can be made a little more efficient (less
getfield opcodes) if written like this
MapN(K key, V value) {
table = new Object[] { key, value };
size = 1;
}
Oops, this was a leftover from my experiment where I adapted MapN to
implement Map1 when setting specializers=0. Removed.
and i do not understand why the field size is not declared @Stable anymore,
ok, it can be equals to zero, but in that case the JIT will emit a move
so it's better than always asking for a move (or i do not understand the
semantics of @Stable ?)
Hmm, I'm under the impression @Stable brings no additional value to a
final non-array fields (definitely important for arrays).
I might have been guilty of adding @Stable to more fields than necessary
in these implementations in the first place. I've
reverted this removal and will add a note to investigate separately if
we can more systematically clean them up.
http://cr.openjdk.java.net/~redestad/8193128/open.01/
Thanks!
/Claes
