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

[forax at univ-mlv.fr]

Hi Stuart,

----- Mail original -----
> De: "Stuart Marks" <stuart.marks at oracle.com>
> À: "Remi Forax" <forax at univ-mlv.fr>
> Cc: "core-libs-dev" <core-libs-dev at openjdk.java.net>
> Envoyé: Mercredi 25 Novembre 2015 01:07:16
> Objet: Re: RFR(m): JEP 269 initial API and skeleton implementation (JDK-8139232)
> 
> 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.

It looks like a bug of javac (or NetBeans),
you should have no warning when you call Arrays.asList().

[...]

> 
> > 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.)

I've not noticed that you want KeyValueHolder to be compatible with the other implementation of Map.Entry,
so you can ignore the paragraph above.

> 
> s'marks
> 

cheers,
Rémi