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

[David Holmes]

Hi Mike,

Looking mainly at the docs not the operational semantics of null 
handling ...

On 17/10/2013 9:52 AM, Mike Duigou 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.

Hmmmm ... see below ...

> http://cr.openjdk.java.net/~mduigou/JDK-8024688/2/webrev/

Map.java:

The implNote for computeIfAbsent should be modified to match the 
implementation. Ditto for computeIfPresent. Ditto for compute, merge 
etc! Once these implementations have stabilized we need to check what 
the implNote says. It makes no sense to me for the impl note to describe 
anything other than the core logic of the actual implementation - 
particularly referring to putIfAbsent when put is used, or replace when 
put is used.

HashMap.java:

1234             if(old.value != null)

Space


ConcurrentMap.java:

  247      * @throws ClassCastException {@inheritDoc}
  248      * @throws NullPointerException {@inheritDoc}
  249      * @throws ClassCastException {@inheritDoc}

CCE is repeated.

  274      * contain null values and get() returning null unambiguously 
means the key

get() should be in code font as per line #69. Ditto for line 300, 332 
and 395.

I now see that those Map implNotes were written with ConcurrentMap in 
mind so that it can just tag on the note about retries. But this seems 
wrong to me - each should have its own implNotes reflecting the true 
implementation.

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

... it still concerns me that a function object could mutate the map and 
so trigger a CCE.

> The patch also fixes up missing @throws and @since from the ConcurrentMap implementations.

I think you have a one-line conflict with Henry's latest @since update.

Cheers,
David

> Mike
>