Hello all. I have updated the webrev with a final correction from Brian Goetz, the @throws NPE didn't reflect that it was thrown unconditionally if value is null.
http://cr.openjdk.java.net/~mduigou/JDK-8029055/2/webrev/ Mike On Nov 25 2013, at 20: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 >>>> >>> >
