RFR(m): JEP 269 initial API and skeleton implementation (JDK-8139232)
Roger Riggs
Roger.Riggs at Oracle.com
Wed Nov 25 21:02:59 UTC 2015
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
More information about the core-libs-dev
mailing list