On Nov 26 2013, at 05:35 , Stephen Colebourne <scolebou...@joda.org> wrote:
> 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".
We tried it a couple of ways. The interpretation of null mapping as "absent"
was the compromise we came up with.
> Similarly, for "computeIfPresent()", my expectation is that the
> mapping is present, and that the null should be an input to the
> function.
This would have meant contradictory interpretations of absent for the different
methods.
> Yes, I get that nulls in collections are a pain, but trying to
> redefine the meaning of "absent" is more of a pain.
The problem is that not all of these methods are new, some are backports from
java.util.concurrent.ConcurrentMap where there were existing null handling
semantics (which didn't take into account null values). Producing consistent
results across implementations seemed most important. This meant favouring the
"null means absent" semantic rather than the "null might mean absent or might
mean a null mapping".
> 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
That seems too surprising.
> - null returned from a function means remove (or is invalid)
> - a null value parameter is invalid
I had chosen to add this rule for merge() in my most recent webrev to reduce
the complexity. It kind of goes against the "null is fully supported as a
value".
> - merge where the existing mapping has a null value should not pass
> the null to the function
This is the same as treating it as absent.
>
> Stephen
>
>
>
> On 26 November 2013 04:32, Mike Duigou <mike.dui...@oracle.com> wrote:
>>
>> On Nov 24 2013, at 16:31 , David Holmes <david.hol...@oracle.com> 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 <henry....@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
>>>>>
>>>>
>>
