See https://bugs.openjdk.java.net/browse/JDK-8029676
Stephen
On 26 November 2013 13:20, Stephen Colebourne <scolebou...@joda.org> wrote:
> I took a quick look, but jumped back in horror at the start of the
> Javadoc for the new methods in Map. A Javadoc description should start
> with the positive, not the negative. I had to read the code to figure
> out what they are supposed to do. Here are the four poor Javadocs and
> some proposed enhacements:
>
> merge()
> "If the specified key is not already associated with ..."
> should be
> "Updates the value for an existing key merging the existing value with
> the specified value."
> "<p>"
> "....more detailed user-level explanation..."
> "<p>"
> "....now the more spec/edge case part..."
> ("if" is a terrible way to start the docs. Say what it does!)
>
>
> computeIfAbsent()
> "If the specified key is not already associated with a value (or is mapped..."
> should be
> "Associates the key with a value computed on demand if the key is not
> currently present."
> "<p>"
> "....more detailed user-level explanation..."
> "<p>"
> "....now the more spec/edge case part..."
>
>
> computeIfPresent()
> "If the value for the specified key is present and non-null,"
> should be
> "Updates the value for an existing key."
> "<p>"
> "....more detailed user-level explanation..."
> "<p>"
> "....now the more spec/edge case part..."
>
>
> compute()
> "Attempts to compute a mapping for the specified key..."
> should be
> "Updates the value for a key, adding a new mapping if necessary."
> "<p>"
> "....more detailed user-level explanation..."
> "<p>"
> "....now the more spec/edge case part..."
> (attempts is negative, not positive)
>
> Similar problems seem to exist for "putIfAbsent".
>
> also...
>
> I have to say that I don't overly like the method name.
> "map.merge(...)" sounds like a bulk operation affecting the whole map
> (ie. like putAll). Assuming that the method cannot be renamed, it
> could be improved just by changing the method parameter names.:
>
> default V merge(K key, V valueToMerge,
> BiFunction<? super V, ? super V, ? extends V> mergeFunction) {
>
> "valueToMerge" implies that action will occur to the parameter.
> "mergeFunction" is much more meaningful that "remapping Function".
>
> Similar parameter name change, "mappingFunction" -> "valueCreationFunction"
> default V computeIfAbsent(
> K key,
> Function<? super K, ? extends V> valueCreationFunction) {
>
> Similar parameter name change, "remappingFunction" -> "valueUpdateFunction"
> default V computeIfPresent(
> K key,
> BiFunction<? super K, ? super V, ? extends V> valueUpdateFunction) {
>
> Similar parameter name change, "remappingFunction" -> "valueUpdateFunction"
> default V compute(
> K key,
> BiFunction<? super K, ? super V, ? extends V> valueUpdateFunction) {
>
>
> also...
> Can you use HashSet::new in the example code?
>
>
> In general, all these new methods are written in a spec style, rather
> than a user friendly style, and I'm afraid I don't think thats
> sufficient.
> 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
>>>>>
>>>>
>>
