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

[Roger Riggs]

Hi Stuart,

Comments:

Set.java:
  - of methods:  @throws IAE   "if the elements are duplicates" -> "if 
elements *contains *duplicates"

  - in the of() methods, would it be more efficient to add the 
individual object to the hashset w/o
    the intermediate list; don't create garbage that needs to be collected.

EnumSet.java:

  - the javadoc for the of(e) method says it is specified by: "

    |of
    <http://cr.openjdk.java.net/%7Esmarks/reviews/jep269/specdiff.20151123/java/util/Set.html#of-E->| in
    interface |Set
    <http://cr.openjdk.java.net/%7Esmarks/reviews/jep269/specdiff.20151123/java/util/Set.html><E
    <http://cr.openjdk.java.net/%7Esmarks/reviews/jep269/specdiff.20151123/java/util/EnumSet.html>
    extends java.lang.Enum<E
    <http://cr.openjdk.java.net/%7Esmarks/reviews/jep269/specdiff.20151123/java/util/EnumSet.html>>>|"
    Must be a javadoc bug because Set is an interface and of is a static 
method.

    This anomaly incorrectly makes EnumSet show up in the diff.


List.java:
  - The use of 'Creates an immutable list...' implies a new List is 
created each time. (ditto @returns "newly created")
    I would use 'Returns an immutable list...'

  - I think I would have gone for 7 or 9 elements in the constructor, 
the 10 and 11 args forms
    just look like they would never be used.  (your comments not with 
standing)

  - in the implementation, it would helpful to use 
Objects.requireNonNull(e1, "e1") form to aid in debugging which arg was 
null.  (Ditto Map, and Set)


Map.java:
  - Might want to clarify that the duplicate check uses equals (not ==).
  - Same comments about the description using "creates/created" and "newly".
  - Map is even stranger than List with 20 arguments (10 arg/value pairs)
  - Capacity and load factor are not relevant for an immutable Map; can 
that be noted somewhere?
  - KeyValueHolder is more general than is needed; it does not need to 
inherit from Entry and could be a nested class in Map.

$,02, Roger



On 11/24/2015 1:26 AM, Stuart Marks wrote:
> 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