Hi Stuart, ----- Mail original ----- > De: "Stuart Marks" <[email protected]> > À: "Remi Forax" <[email protected]> > Cc: "core-libs-dev" <[email protected]> > 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
