Sorry for my delayed review. Basically, I'm happy with this. Some oddments:
Map.ofEntries(Entry...) "The entry objects themselves are not stored in the map." This seems wrong. `Entry` might be implemented as a value, not an object in the future. And if so, it might form part of the state of an optimised list implementation. The use of <K,V> looks like it should have an extra space after the comma to me. I prefer to avoid apostrophes in docs "the map's key type" -> "the key type of the map" "the first mapping's key" -> "the key of the first mapping" I note that java.util.Stream uses empty() but this API uses of() for the empty case. I can see both points of view and don't have much of a preference. Is consistency with Stream needed? KeyValueHolder needs a <p> at line 41 thanks Stephen On 4 December 2015 at 00:58, Stuart Marks <stuart.ma...@oracle.com> wrote: > Small refresh here: after some consultation with Brian Goetz and John Rose, > I've updated the class doc text covers immutability and value-based. They > now say, > > * They are structurally immutable. Elements cannot be added, removed, or > replaced. Attempts to do so result in UnsupportedOperationException. > However, if the contained elements are themselves mutable, this may cause > the List's contents to appear to change. > > [and similar for Set and Map] > > * They are value-based. Callers should make no assumptions about the > identity of the returned instances. Factories are free to create new > instances or reuse existing ones. Therefore, identity-sensitive operations > on these instances (reference equality (==), identity hash code, and > synchronization) are unreliable and should be avoided. > > -- > > I still need an official OpenJDK Reviewer. > > -- > > API: > > http://cr.openjdk.java.net/~smarks/reviews/jep269/api.20151203/ > > Specdiff: > > > http://cr.openjdk.java.net/~smarks/reviews/jep269/specdiff.20151203/overview-summary.html > > Webrev: > > http://cr.openjdk.java.net/~smarks/reviews/jep269/webrev.20151203/ > > Thanks, > > s'marks > > > On 12/1/15 6:35 PM, Stuart Marks wrote: >> >> Hi all, >> >> Thanks for the previous round of review comments. Here's an updated API >> and >> implementation for review. >> >> I've run specdiff with the --hu ("hide unchanged") option, so only the >> bits of >> the specification that have changed are shown. As before, though, please >> ignore >> the spurious change to EnumSet caused by a javadoc bug. >> >> API changes: >> - add clarifying notes on immutability >> - remove wording that implied creation of new objects >> - add "value-based" disclaimers >> - add ordering specification for List and non-ordering disclaimers >> for Set and Map >> - clarify that Map.ofEntries() doesn't store the Map.Entry objects, >> instead >> it extracts keys and values >> - Map.Entry instances returned from Map.entry() are *not* serializable >> >> Other: >> - markup cleanups >> - small implementation cleanups >> >> JEP: >> >> http://openjdk.java.net/jeps/269 >> >> API spec (basically List, Map, and Set): >> >> http://cr.openjdk.java.net/~smarks/reviews/jep269/api.20151201/ >> >> Specdiff: >> >> >> >> http://cr.openjdk.java.net/~smarks/reviews/jep269/specdiff.20151201/overview-summary.html >> >> >> Webrev: >> >> http://cr.openjdk.java.net/~smarks/reviews/jep269/webrev.20151201/ >> >> Thanks, >> >> s'marks
