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

[Stuart Marks]

On 11/25/15 1:02 PM, Roger Riggs wrote:
> Set.java:
>   - of methods:  @throws IAE   "if the elements are duplicates" -> "if elements
> *contains *duplicates"

That specific wording comes from the two-arg overload, so it's shorthand for "if 
the (two) elements are duplicates (of each other)". The overloads with more args 
are worded "if there are any duplicate elements" which I think is ok; similar 
wording is used in Map.

>   - 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.

Sure. I'll consider this for the optimized implementation, since all of this 
code is going to replaced at some point.

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

Yes, this is a javadoc bug, https://bugs.openjdk.java.net/browse/JDK-8139101 .

> 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...'

Agreed, will fix here, in the @return clauses, and in corresponding locations 
for Set and Map.

>   - 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)

Will reconsider for the optimized implementation.

> Map.java:
>   - Might want to clarify that the duplicate check uses equals (not ==).

The general contracts of Set and Map talk about duplicates and the use of 
equals(), so I think we're covered here.

>   - 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?

The concepts of capacity and load factor are defined by HashMap, so they aren't 
present in the Map interface at all, irrespective of these changes. The 
discussion on this topic earlier in the thread is about the HashMap capacity 
values used internally within the skeleton implementation, which doesn't affect 
the API.

>   - KeyValueHolder is more general than is needed; it does not need to inherit
> from Entry and could be a nested class in Map.

KeyValueHolder is a private class; interfaces can't have nested private classes. 
(The "Milling Project Coin" feature [1] was adding private *methods* to 
interfaces.) The new Map.entry() method's return type is Map.Entry and it 
returns a KeyValueHolder, which therefore must implement Map.Entry. 
KeyValueHolder was a public class in a previous version of this proposal. After 
a suggestion from Stephen Colebourne, and some consultation with Brian Goetz, we 
were able to convince ourselves that having KVH be a private class implementing 
Map.Entry would still allow us to get the benefit of turning it into a value 
type in the future.

s'marks


[1] http://openjdk.java.net/jeps/213