RFR: 8029055: Map.merge @implDoc is incorrect. (was: Map.merge Javadoc is confusing)

[Henry Jen]

+1 (Not a reviewer).

Cheers,
Henry

On Fri 22 Nov 2013 04:25:50 PM PST, 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
>>
>