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

[Mike Duigou]

Thank you for the feedback. Responses inline.

On Oct 22 2013, at 18:45 , David Holmes <david.holmes at oracle.com> wrote:

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

Yep, I generated it for the multiplatform test run.

> 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

I believe this was made an implSpec because of the "Implementations which support null values *must* override this default implementation." sentence but you are right that it does read more like an @implNote. 

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

Both sound more like @implNote to me. The halcyon days after ZBB Stuart and I can finally finish the promised

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

You are, of course, correct. Reviewing the HTML javadoc this extends to Hashtable, ConcurrentHashMap, IdentityHashMap, and all the Collections.XXXMap and probably other places. I would like to tackle this as a separate issue since it's very widespread. I have filed https://bugs.openjdk.java.net/browse/JDK-8027125 to address this. 

I fear that with the proper tooling to locate missing @throws in overridden impls we could generate many bugs of this type....

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