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
