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

Reply via email to