RFR(m): JEP 269 initial API and skeleton implementation (JDK-8139232)

[Stuart Marks]

On 11/24/15 7:03 AM, Remi Forax wrote:
> The other way to detect collision instead of checking the size if to use the return value of put, it should be null if it's a new key,
> but the code is maybe less readable.

Yes. This is just a "skeleton" implementation, so I'm not terribly interested in 
making a bunch of improvements to the code. But if this code does end up 
surviving into the final release, we should definitely take a closer look at 
some of these details.

> There are several instance of
>     @SafeVarargs
>     @SuppressWarnings("varargs")
> if you use @SafeVarargs you should not to use @SuppressWarnings("varargs").

These cover different cases. The @SafeVarargs annotation is a declaration to the 
caller that this method doesn't cause heap pollution, so that callers of this 
method don't get a warning.

The @SuppressWarnings("varargs") is to suppress a warning within this method at 
the call to Arrays.asList(), which does issue a varargs warning. (This despite 
the fact that Arrays.asList() is annotated with @SafeVarargs. Hmm.)

I should move the @SuppressWarnings to the right spot within the method, though, 
as it doesn't need to apply to the entire method.

> In Map.ofEntries, calling Objects.requireNonNull on the entry inside the for loop is not necessary given getKey is called on that entry.

Yes, also noted by Paul Benedict.

> For the Set, i think i prefer to avoid the call to Array.asList and use add instead,
>    ...
> and Set.of(E...) can be simply written like this:
>    ...

As above, I don't want to spend too much time on the implementation right now as 
it's not the "real" implementation. The plan is to replace this entirely with 
compacte, optimized implementations. That's where the micro-optimization reviews 
should come in. :-)

> For KeyValueHolder, in equals, you don't need to access to the getter here,
>    return key.equals(e.key) && value.equals(e.value);

In this case e is Map.Entry<?,?> which could be an instance of one of the other 
Map.Entry implementations, not necessarily a KeyValueHolder, so this needs to 
use getKey() and getValue().

> and if you can fix something that baffle me with the usual implementations of Map.Entry,
> a good hashCode for KeyValueHolder should have the property that hashCode for entry(a, b) is different form the hashCode of entry(b, a),
> something like:
>    return key.hashCode() ^ Integer.rotateLeft(value.hashCode(), 16);
> or any variations.

Sigh. This hashCode computation is specified in Map.Entry.hashCode() and is 
implemented this way by AbstractMap.SimpleEntry/SimpleImmutableEntry. I think 
KeyValueHolder should match. Sorry.

(Note also that KeyValueHolder is no longer a public class.)

s'marks