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

[Remi Forax]

Hi Stuart,

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.

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

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

For the Set, i think i prefer to avoid the call to Array.asList and use add instead,
  HashSet<E> set = new HashSet<>(3);
  set.add(Objects.requireNonNull(e1));
  set.add(Objects.requireNonNull(e2)));
  if (set.size() != 2) {
    throw new IllegalArgumentException("duplicate elements");
  }
  return Collections.unmodifiableSet(set);
  
and Set.of(E...) can be simply written like this:
  @SafeVarargs
  static <E> Set<E> of(E... es) {
    HashSet<E> set = new HashSet<>((int) (c.size()/.75f));
    for (E e : es) { // throws NPE if es is null
      Objects.requireNonNull(e);
      set.add(e);
    }
    if (set.size() != es.length) {
      throw new IllegalArgumentException("duplicate elements");
    }
    return Collections.unmodifiableSet(set);
  }

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

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.

cheers,
Rémi

----- Mail original -----
> De: "Stuart Marks" <stuart.marks at oracle.com>
> À: "core-libs-dev" <core-libs-dev at openjdk.java.net>
> Envoyé: Mardi 24 Novembre 2015 07:26:27
> Objet: RFR(m): JEP 269 initial API and skeleton implementation (JDK-8139232)
> 
> Hi all,
> 
> Please review these API and implementation changes that comprise the first
> integration of JEP 269. I intend to push this under the "initial API and
> skeleton implementation" subtask, JDK-8139232.
> 
> Changes since the previous review:
>   - more precise wording regarding static factory methods (thanks Remi)
>   - add defensive copy and test for List.of(E...) (thanks Michael Hixson)
>   - more null checking and tests
>   - various small wording cleanups
>   - @since 9
> 
> I've left the fixed-arg overloads at ten for all three interfaces. The
> fixed-arg
> methods provide a useful fast path for initialization and non-desktop,
> non-server cases. The cost is some API clutter; ten elements or pairs is
> rather
> a lot. This number should be sufficient, though, to handle the vast majority
> of
> cases without having to switch to varargs. Note that the clutter is only in
> the
> API definitions, and it doesn't intrude into the point of call (at least for
> List and Set). For the Map case, it's particularly useful to have lots of
> fixed-arg overloads, since the varargs case requires switching APIs and
> adding
> more boilerplate.
> 
> I've also updated the JEP text to reflect the current proposal, mainly the
> removal of the static factory methods from the concrete classes.
> 
> JEP:
> 
> 	http://openjdk.java.net/jeps/269
> 
> API spec (basically List, Map, and Set):
> 
> 	http://cr.openjdk.java.net/~smarks/reviews/jep269/api.20151123/
> 
> Specdiff:
> 
> 	http://cr.openjdk.java.net/~smarks/reviews/jep269/specdiff.20151123/overview-summary.html
> 
> Webrev:
> 
> 	http://cr.openjdk.java.net/~smarks/reviews/jep269/webrev.20151123/
> 
> Thanks,
> 
> s'marks
>