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

[David Holmes]

Hi Mike,

On 19/10/2013 5:34 AM, Mike Duigou wrote:
> Updated webrev:
>
> http://cr.openjdk.java.net/~mduigou/JDK-8024688/3/webrev/

I see there is a version 4 now :)

ConcurrentMap:

In replaceAll:

  261      * <p>This implementation assumes that the ConcurrentMap 
cannot contain null ...

is this intended to be implSpec or implNote? Currently it appears to be 
implSpec. Ditto for the same text in compute*, merge

Generally is the use of "this implementation" versus "the default 
implementation" significant? I thought perhaps the former was for 
implNotes and the latter implSpec ?

----

HashMap:

None of the overriding implementations have any @throws for unchecked 
exceptions. Some of them may no longer apply but I'm not sure they all 
do - eg ClasscastException.

Otherwise I have no further comments. :)

Thanks,
David

> More notes below.
>
> On Oct 16 2013, at 21:20 , David Holmes <david.holmes at oracle.com> wrote:
>
>> Hi Mike,
>>
>> 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.
>
> While removing the CME I opted to also remove some of use of the fancy new methods as they didn't add anything and (usually) had a performance cost.
>
> I will update the @implNote descriptions and review the other notes.
>
>>
>> HashMap.java:
>>
>> 1234             if(old.value != null)
>>
>> Space
>
> Got it. Thank you.
>
>>
>>
>> ConcurrentMap.java:
>>
>> 247      * @throws ClassCastException {@inheritDoc}
>> 248      * @throws NullPointerException {@inheritDoc}
>> 249      * @throws ClassCastException {@inheritDoc}
>>
>> CCE is repeated.
>
> Got it.
>
>>
>> 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.
>
> And a few more. Thanks.
>
>>
>> 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.
>
> There seems to be no way to prevent it.
>
>>
>>> 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.
>
> Got it. I had planned for that fix to go in with changeset....
>
>>
>> Cheers,
>> David
>>
>>> Mike
>>>
>