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
>>>>
>>>
>
