RFR: 8024688: j.u.Map.merge doesn't work as specified if contains key:null pair

[Mike Duigou]

On Oct 17 2013, at 07:36 , Paul Sandoz <paul.sandoz at oracle.com> wrote:

> 
> On Oct 17, 2013, at 1:52 AM, Mike Duigou <mike.duigou at oracle.com> wrote:
> 
>> 
>> On Oct 16 2013, at 05:34 , Paul Sandoz <paul.sandoz at oracle.com> wrote:
>> 
>>> On Oct 16, 2013, at 1:52 PM, David Holmes <david.holmes at oracle.com> wrote:
>>>>>> Perhaps HashMap's implementations should throw CME?
>>>>>> 
>>>>> 
>>>>> Perhaps, seems to be going beyond the call of duty. My inclination is not to bother. It becomes most relevant with forEach since the consumer will have side-effects that might make it easier to unintentionally slip in a modification to the map itself.
>>>> 
>>>> I think there is a lot to be said for consistency.
>>> 
>>> Yes, i was proposing to consistently not support it for non-traversal methods :-)
>> 
>> 
>> I have prepared an updated webrev removing all of the non-traversal CME throwing.
>> 
>> http://cr.openjdk.java.net/~mduigou/JDK-8024688/2/webrev/
>> 
> 
> Code looks good.
> 
> Agree with David there is still some work to do cleaning up the doc.
> 
> e.g.:
> 
> On concurrent map this:
> 
>     * @implNote This implementation assumes that the ConcurrentMap cannot
>     * contain null values and {@code get()} returning null unambiguously means
>     * the key is absent. Implementations which support null values
>     * <strong>must</strong> override this default implementation.
> 
> and this variant of the same thing:
> 
>     * The default implementation assumes that the ConcurrentMap cannot
>     * contain null values and get() returning null unambiguously means the key
>     * is absent. Implementations which support null values
>     * <strong>must</strong> override this default implementation.
> 
> should be impl specs not notes. (It could also be pulled up into the class rather than repeating for each default method.) May get reformulated if you state "The default implementation is equivalent to...".

Done.

> 
> For:
> 
>    /**
>     * {@inheritDoc}
>     *
>     * @implNote The default implementation may retry these steps when multiple
>     * threads attempt updates.
>     *
>     * The default implementation assumes that the ConcurrentMap cannot
>     * contain null values and get() returning null unambiguously means the key
>     * is absent. Implementations which support null values
>     * <strong>must</strong> override this default implementation.
>     *
>     * @throws UnsupportedOperationException {@inheritDoc}
>     * @throws ClassCastException {@inheritDoc}
>     * @throws NullPointerException {@inheritDoc}
>     * @since 1.8
>     */
>    @Override
>    default V computeIfAbsent(K key,
>            Function<? super K, ? extends V> mappingFunction) {
>        Objects.requireNonNull(mappingFunction);
>        V v, newValue;
>        return ((v = get(key)) == null &&
>                (newValue = mappingFunction.apply(key)) != null &&
>                (v = putIfAbsent(key, newValue)) == null) ? newValue : v;
>    }
> 
> there is no retry.

Fixed

>> It does bother me to be throwing out "good information" by not throwing the CMEs but I'm willing to go with the flow. As a practical matter later reintroduction of even valid error detection would almost certainly be difficult. (https://bugs.openjdk.java.net/browse/JDK-5045147 for one example).
>> 
> 
> I still think we can solve that with a statement on Map (my preference is to be DRY). An impl supporting CME should override the defaults, which is not that different conceptually to what is said about synchronization/atomicity on Map or null values on ConcurrentMap.

I haven't tried to address this. My inclination is to instead document the behaviour or the implementing classes. I don't like the notes in Map referring to what ConcurrentMap impls must do.

Mike