RFR: 8004518 & 8010122 : Default methods on Map

[Mike Duigou]

On Apr 15 2013, at 00:39 , Peter Levart wrote:

> Hi Mike,
> 
> Another thing to note: Some new methods in HashMap need to call inflateTable(), since patch for 8011200 has already been pushed to tl...

Yes. I actually had an off-to-the-side patch which added these and tested with it last week. The missing checks are now in lambda repo and part of the 8010122 patch.

Thanks.

Mike

> 
> Regards, Peter
> 
> On 04/14/2013 08:35 PM, Peter Levart wrote:
>> 
>> 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
>>> 
>> 
>