On Apr 8 2013, at 19:22 , David Holmes wrote: > Hi Mike, > > Looking only at Map itself for now. > > On 9/04/2013 4:07 AM, Mike Duigou wrote: >> Hello all; >> >> This is a combined review for the new default methods on the java.util.Map >> interface being added for the JSR-335 lambda libraries. The reviews are >> being combined because they share a common unit test. >> >> http://cr.openjdk.java.net/~mduigou/JDK-8010122/0/webrev/ > > General style issues: > > - spaces after keyword ie "if (x == null)" not "if(x == null)"
Fixed. I am sorry this keeps coming up. I am loathe to run an automatic formatter on any JDK code. > - comparisons against constants/null put the constant/null on the right ie > "if (x == null)" not "if (null == x)" Do I shall. > > (where has our style guide gone? I can't find it on internal or external > wikis :( ) This one? http://www.oracle.com/technetwork/java/codeconv-138413.html I am unaware of any other guide that's official policy. > >> 8004518: Add in-place operations to Map >> forEach() >> replaceAll() > > Both of those contain the boilerplate text: > > * <p>The default implementation makes no guarantees about > * synchronization or atomicity properties of this method. Any > * class overriding this method must specify its concurrency > * properties. In particular, all implementations of > * subinterface {@link java.util.concurrent.ConcurrentMap} > * must ensure that this operation is performed atomically. > > but these methods are not, and can not be, atomic in ConcurrentMap I've altered the text to incorporate your suggestions. It now read: * <p>The default implementation makes no guarantees about synchronization * or atomicity properties of this method. Any class which wishes to provide * specific synchronization, atomicity or concurrency behaviour should * override this method. I would like to use this same text on the other methods as well. > > forEach and replaceAll are very similar in terms of taking and applying a > "operation" to each entry, yet their descriptions use a completely different > style. Written by different people independently. > forEach describes thing in general terms while replaceAll talks about calling > the functions' apply method with the current entry's key and value. I would > suggest for replaceAll: > > "Replaces each entry's value with the result of invoking the given function > on that entry, in the order entries are returned by an entry set iterator, > until all entries have been processed or the function throws an exception." > > This is also makes "replace" the subject rather than "apply". Yes, reads better to me. > > forEach doesn't declare the IllegalStateException that getKey and getValue > can throw. Since forEach isn't supposed to mutate the map this shouldn't happen. It could only happen through concurrent modification. I've added a catch of the IllegalStateException and throw CME with the ISE as the cause to both forEach and replaceAll. > Some/many of the @throws text has obviously been copied from the Map.Entry > methods eg: > > * @throws ClassCastException if the class of the specified value > * prevents it from being stored in the backing map > > but when put into Map itself they should not be referring to "the backing > map" as there is no "backing map". Further we have inconsistent terminology > being used, eg getOrDefault has: > > * @throws ClassCastException if the key is of an inappropriate type for > * this map > > and then there is a third variant in other methods: > > * @throws ClassCastException if the class of the specified key or value > * prevents it from being stored in this map > * (<a href="Collection.html#optional-restrictions">optional</a>) > > These should all have the same basic wording, differing only in key/value. agreed. The third variant is now used consistently. > >> 8010122: Add atomic operations to Map > > Meaning "backport some operations from ConcurrentMap" - they aren't actually > atomic in Map. OK. > >> getOrDefault() > > No comment > >> putIfAbsent() * > > The default implementation throws ConcurrentModificationException but this is > not declared in the spec. fixed > >> remove(K, V) >> replace(K, V) >> replace(K, V, V) > > No comments > >> compute() * >> merge() * >> computeIfAbsent() * >> computeIfPresent() * > > The following generally apply to this group of methods. > > As Peter already stated the spec: > > * <p>If the function returns {@code null}, the mapping is removed (or > * remains absent if initially absent). > > is somewhat unclear. The parenthesized part is not connected to the function > returning null or otherwise; as the function won't be called. Corrected. > I find the spec for these rather confusing from a concurrency perspective - > this non-concurrent interface seems to be trying to say too much about how a > concurrent interface should specify behaviour. Why does it need to say: > > * In concurrent contexts, the default implementation may retry > * these steps when multiple threads attempt updates. For default on Map I am starting to think that throwing that CME rather than looping is the right choice. The retry behaviour seems to be counter the basic behaviour of non-concurrent implementations. The retry behaviour will just hide usage that's otherwise unsafe when used with non-concurrent implementations. The concurrent implementations don't use these defaults so there's no problem switching to throwing CME. Opinions? > ? Note computeIfAbsent does not say the same thing. It doesn't attempt to loop. > > The @implSpec does not match the actual implementation. It looks to me like > these implementations are trying to cater for concurrent situations - hence > the loop. That's okay but then the implSpec should identify that fact. Corrected several of the impl descriptions. > 980 * be of use when combining multiple mapped values for a key. For > 981 * example. to either create or append a {@code String msg} to a > > Period after example should be a comma. fixed. > > Cheers, > David > >> The * operations treat null values as being absent. (ie. the same as there >> being no mapping for the specified key). >> >> The default implementations provided in Map are overridden in HashMap for >> performance purposes, in Hashtable for atomicity and performance purposes >> and in Collections for atomicity. >> >> Mike >> >>
