RFR(m): 8139233u1 add initial compact immutable collection implementations

Stuart Marks stuart.marks at oracle.com
Wed May 11 01:25:15 UTC 2016



On 5/9/16 7:25 AM, Remi Forax wrote:
> I still think that the code of hasNext of the iterator of SetN and MapN should not do any side effect.

Yes, I'm happy to consider further cleanups. I was starting to go off into the 
weeds with refactoring and cleanups, since they were introducing regressions, so 
I decided to check in a stable version and continue with cleanups later.

You had suggested to gather the searching code into a nextIndex() method, which 
is called from the constructor and from next(), in order to simplify hasNext(). 
I see that HashMap's HashIterator is done this way, and in fact the search code 
isn't in a method -- it's replicated between the constructor and nextNode(). See 
HashMap.java lines 1477 and 1493. [1] [2]

My question is, does this really buy anything? In a typical 
while(hasNext())/next() loop, the same work will get done, just in a different 
place.


[1] 
http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/4da0f73ce03a/src/java.base/share/classes/java/util/HashMap.java#l1477

[2] 
http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/4da0f73ce03a/src/java.base/share/classes/java/util/HashMap.java#l1493


> otherwise,
>   in the constructor of MapN, you can declare k and v as Object
>     (the table is an array of Object and probe() now takes an Object)

Yes, good point.

>    also instead of InternalError, i think AssertionError is a better exception,
>     the javadoc of InternalError says that InternalError is thrown by the VM.

Hm. I agree that InternalError seems wrong since it's a subtype of 
VirtualMachineError. But it's used about 2x more frequently than AssertionError 
in the JDK.

>   in MapN.probe, you can remove the cast to K (and the corresponding @SuppressWarnings).

Yes.

>   Set2.e0 and Set2.e1 should be declared as package-private otherwise, the compiler generates an accessor in the iterator. Same thing for SetN.elements, MapN.table and MapN.size

I guess I privatized too many things. (But I'm no David Cameron.)

>   CollSer implementation can be improved to avoid the ugly switch/case by replacing the constant list by an enum,
>   something like the code below. In that case, creating a CollSer for a Map will be done like this,
>     return new CollSer(CollSer.Kind.MAP, array);

I'll reply downthread after your exchange with Stephen Colebourne.

s'marks



More information about the core-libs-dev mailing list