Another updated webrev:
http://cr.openjdk.java.net/~mduigou/JDK-8010122/6/webrev/
I've started pre-integration final testing on this patch and plan to push it
tomorrow unless something significant comes up.
Mike
On Apr 15 2013, at 14:31 , Mike Duigou wrote:
>
> 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
>>>
>>
>
