RFR: 8029055: Map.merge @implDoc is incorrect.

[David Holmes]

Hi Mike,

There is still no clear spec for what should happen if the param value 
is null. To me the spec implies that null should become the new value 
for the mapping, but the code actually removes the mapping.

There is also a further error in this spec. First we have:

1112      * If the specified key is not already associated with a value 
or is
1113      * associated with null, associates it with the given value.
1114      * Otherwise, replaces the value with the results of the given
1115      * remapping function

but then we have

1124      * <p>If the function returns {@code null}, the mapping is 
removed (or
1125      * remains absent if initially absent).

The parenthesized part is wrong. If the mapping was initially absent 
then the function is never even examined so we don't know if it returns 
null or not. But let us assume that it would have returned null. If the 
mapping was initially absent then we create a new mapping from 
key->value, hence regardless of whether the function would return null, 
the mapping does not remain absent if initially absent.

Also you have changed the method implementation not just the implDoc so 
the bug synopsis is somewhat misleading.

Thanks,
David

On 23/11/2013 10:25 AM, Mike Duigou wrote:
> We'll be using https://bugs.openjdk.java.net/browse/JDK-8029055 for this issue.
>
> I've posted a webrev here:
>
> http://cr.openjdk.java.net/~mduigou/JDK-8029055/0/webrev/
>
> There is an identical change in ConcurrentMap's merge().
>
> Mike
>
> On Nov 22 2013, at 16:01 , Henry Jen <henry.jen at oracle.com> wrote:
>
>>
>> On 11/21/2013 06:33 PM, David Holmes wrote:
>>> On 22/11/2013 5:02 AM, Louis Wasserman wrote:
>>>> While I agree that case should be specified, I'm not certain I follow why
>>>> that's what's going on here.
>>>>
>>>> The weird condition is that if oldValue is null, not value; oldValue
>>>> is the
>>>> old result of map.get(key).  The Javadoc seems not just unspecified, but
>>>> actively wrong.  Here's a worked example:
>>>>
>>>> Map<String, Integer> map = new HashMap<>();
>>>> map.merge("foo", 3, Integer::plus);
>>>> Integer fooValue = map.get("foo");
>>>>
>>>> Going through the Javadoc-specified default implementation:
>>>>
>>>>    V oldValue = map.get(key); // oldValue is null
>>>>    V newValue = (oldValue == null) ? value :
>>>>                 remappingFunction.apply(oldValue, value);
>>>>       // newValue is set to value, which is 3
>>>>    if (newValue == null) // newValue is nonnull, branch not taken
>>>>        map.remove(key);
>>>>    else if (oldValue == null) // oldValue is null, branch taken
>>>>        map.remove(key); // removes the entry from the map
>>>>    else // else if was already triggered, branch not taken
>>>>        map.put(key, newValue);
>>>>
>>
>> Seems like a document bug to me, we should fix this @implSpec.
>>
>> Cheers,
>> Henry
>>
>