On Apr 14 2013, at 11:35 , 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)...
In the current patch I've used:
default boolean remove(Object key, Object value) {
Object curValue = get(key);
if (!Objects.equals(curValue, value) ||
(curValue == null && !containsKey(key))) {
return false;
}
remove(key);
return true;
}
and similar for replace(K,V,V)
I rearranged it mostly so that calling containsKey is a last resort.
Mike
>
> 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
>>
>
