RFR: 8004518 & 8010122 : Default methods on Map

[Peter Levart]

On 04/14/2013 07:54 PM, Peter Levart wrote:
> Hi Mike,
>
> Just a nit: The order of boolean sub-expressions in Map.replace(key, 
> oldValue, newValue):
>
>   740         if (!containsKey(key) || !Objects.equals(get(key), oldValue))
>
> ...would be more optimal if reversed (like in Map.remove(key, value)).
>
> Regards, Peter

I think the most optimal is actually this:

default boolean remove((Object key, Object value) {
     Object curValue = get(key);
     if (curValue == null && (value != null || !containsKey(key)) ||
         curValue != value && !curValue.equals(value)) {
         return false;
     }
     remove(key);
     return true;
}

...and the same for replace(key, oldValue, newValue)...

Regards, Peter

>
> On 04/13/2013 12:02 AM, Mike Duigou wrote:
>> I have updated the webrev with these changes and a few more.
>>
>> merge() required an update to it's specification to correctly account for null values.
>>
>> http://cr.openjdk.java.net/~mduigou/JDK-8010122/5/webrev/
>>
>> This version is currently undergoing final pre-integration testing. Unless additional problems are found or the testing fails this version will be integrated.
>>
>> Mike
>>
>> On Apr 12 2013, at 12:53 , Mike Duigou wrote:
>>
>>> Thanks for the corrections. I have incorporated all of these into the integration version of the patch.
>>>
>>> On Apr 12 2013, at 12:50 , Akhil Arora wrote:
>>>
>>>> Hi Mike,
>>>>
>>>> a few small things -
>>>>
>>>> UnmodifiableMap.forEach
>>>> is missing Objects.requireNonNull(action);
>>> Added.
>>>
>>>> EmptyMap.replaceAll(BiFunction)
>>>> should just return instead of throwing UnsupportedOpEx
>>>> particularly since EmptyList.replaceAll also returns silently
>>>> after checking if function is null to fulfil the NPE contract
>>> I agree.
>>>
>>>> Hashtable
>>>> is missing a synchronized override of forEach
>>> added.
>>>
>>>> CheckedMap.typeCheck(BiFunction)
>>>> is missing from the patch (won't compile without it)
>>> Apparently I forgot a qrefresh before generating the webrev. I had this in my local copy as it's necessary.
>>>
>>>> On 04/11/2013 01:03 PM, Mike Duigou wrote:
>>>>> Another revision incorporating primarily documentation feedback.
>>>>>
>>>>> http://cr.openjdk.java.net/~mduigou/JDK-8010122/2/webrev/
>>>>>
>>>>> I've also included the java.util.Collections overrides for the default methods. All of these are performance enhancements--the semantics were already correct because the defaults use only public methods.
>>>>>
>>>>> This is likely, modulo formatting of the source examples, the version that will be pushed to TL on Friday unless somebody squawks.
>>>>>
>>>>> Moving the current compute, computeIfPresent, computeIfAbsent and merge defaults to ConcurrentMap and replacing the current Map defaults with versions that throw ConcurrentModificationException will likely be resolved in a future issue if the EG determines it to be important.
>>>>>
>>>>> Mike
>>>>>
>>>>>
>>>>> On Apr 10 2013, at 22:42 , Mike Duigou wrote:
>>>>>
>>>>>> I've posted an updated webrev with the review comments I have received.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~mduigou/JDK-8010122/1/webrev/
>>>>>>
>>>>>> One important point to consider:
>>>>>>
>>>>>> - The current implementations of compute, computeIfPresent, computeIfAbsent, merge are implemented so that they can work correctly for ConcurrentMap instances. For non-concurrent implementations it might be better to fail and throw ConcurrentModification exception whenever concurrent modification is detected. For regular Map implementations the retry behaviour will only serve to mask errors.
>>>>>>
>>>>>> Thoughts?
>>>>>>
>>>>>> Mike
>>>>>>
>>>>>> On Apr 8 2013, at 11:07 , 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/
>>>>>>>
>>>>>>> 8004518: Add in-place operations to Map
>>>>>>> forEach()
>>>>>>> replaceAll()
>>>>>>>
>>>>>>> 8010122: Add atomic operations to Map
>>>>>>> getOrDefault()
>>>>>>> putIfAbsent()          *
>>>>>>> remove(K, V)
>>>>>>> replace(K, V)
>>>>>>> replace(K, V, V)
>>>>>>> compute()              *
>>>>>>> merge()                *
>>>>>>> computeIfAbsent()      *
>>>>>>> computeIfPresent()     *
>>>>>>>
>>>>>>> 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
>