See the new thread for the general Javadoc issues. I'll focus on null here.
Given a map {"Key" -> null} I find the notion that putIfAbsent() or
computeIfAbsent() ignore the existing mapping just plain wrong. While
I can rationalise it (just) it is a horrible reworking of the meaning
of "absent".
Similarly, for "computeIfPresent()", my expectation is that the
mapping is present, and that the null should be an input to the
function.
Yes, I get that nulls in collections are a pain, but trying to
redefine the meaning of "absent" is more of a pain. I'm aware that
ConcurrentMap makes things interesting, but these two just look wrong.
So, my rules would be
- if an existing mapping has a null value, then the mapping is
"present" and not "absent" (only affects methods with those words in
their name)
- none of these methods should try to add/update a mapping with a null value
- null returned from a function means remove (or is invalid)
- a null value parameter is invalid
- merge where the existing mapping has a null value should not pass
the null to the function
Stephen
On 26 November 2013 04:32, Mike Duigou <[email protected]> wrote:
>
> On Nov 24 2013, at 16:31 , David Holmes <[email protected]> wrote:
>
>> Hi Mike,
>>
>> There is still no clear spec for what should happen if the param value is
>> null.
>
> I feel very uncomfortable the status quo of with null being ignored, used for
> a sentinel and also as value. The relations between null and values in this
> method are just too complicated.
>
> Currently:
>
> - The provided value may be either null or non-null. Is null a legal value?
> It depends upon:
> - Is there an existing value?
> - Does the Map allow null values?
> - Does the function allow null values?
> - Existing null values are treated as absent.
> - If a null value is passed should we remove this mapping or add it
> to the map?
> - null might not be accepted by the map
> - The associated value would still be regarded as absent for
> future operations.
> - The function may return null to signal "remove".
>
> In particular I dislike adding a null entry to the map if there is no current
> mapping (or mapped to null). It seems like it should be invariant whether we
> end up with an associated value. If null is used to signify "remove" then
> map.contains(key) will return variable results depending upon the initial
> state. Having the behaviour vary based upon whether the map allows null
> values would be even worse.
>
> So I would like to suggest that we require value to be non-null. I have
> provisionally updated the spec and impls accordingly.
>
>> The parenthesized part is wrong.
>
> I think that's overzealous copying from compute(). I have removed it.
>
>>
>> Also you have changed the method implementation not just the implDoc so the
>> bug synopsis is somewhat misleading.
>
> I will correct this. More changes were made than I originally expected. New
> synopsis will be "Map.merge implementations should refuse null value param"
>
> I have updated the webrev.
>
> http://cr.openjdk.java.net/~mduigou/JDK-8029055/1/webrev/
>
> Thanks,
>
> Mike
>
>>
>> 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 <[email protected]> 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
>>>>
>>>
>
