RFR 8071667 : HashMap.computeIfAbsent() adds entry that HashMap.get() does not find.

[Brent Christian]

Hi, Paul

Thank you for the suggested doc adjustments.  They're applied here:
http://cr.openjdk.java.net/~bchristi/8071667/webrev.3/

TestNG test update forthcoming.

-Brent

On 3/19/15 3:09 AM, Paul Sandoz wrote:
> Hi Brent,
>
> The implementation looks good.
>
> Documentation-wise i think it needs some adjustment:
>
> - the @apiNote should be promoted to "normal" documentation as we want to make a stronger statement.
>
> - I think the @implNote on Map should be merged into the @implSpec. How about:
>
>    <p>The default implementation makes no guarantees about detecting if the mapping function modifies
>    this map during computation and, if appropriate, reporting an error.
>    Non-concurrent implementations should override this method and, on a best-effort basis, throw a
>    {@code ConcurrentModificationException} if it is detected that the mapping function modifies this
>    map during computation.
>    Concurrent implementations should override this method and, on a best-effort basis, throw a
>    {@code IllegalStateException} if it is detected that the mapping function modifies this map
>    during computation and as a result computation would never complete.
>
> ?
>
> - the @implSpec on HashMap etc. should be promoted to "normal" documentation:
>
>    <p>This method will, on a best-effort basis, throw a {@link ConcurrentModificationException} if it is detected
>    that the mapping function modifies this map during computation.
>
> ?
>
> Testing-wise can you use a TestNG data supplier? then you can separate out the larger test into the smaller tests. Meaning we test all combinations regardless of which fail, and each will be reported so there is no need to print out test info. If you need help with that i can provide an example.
>
> Thanks,
> Paul.
>
> On Mar 18, 2015, at 6:46 PM, Brent Christian <brent.christian at oracle.com> wrote:
>
>> Hi,
>>
>> Please review my changes for 8071667 : "HashMap.computeIfAbsent() adds entry that HashMap.get() does not find"
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8071667
>>
>> Webrev+specdiff:
>> http://cr.openjdk.java.net/~bchristi/8071667/webrev.2/
>>
>>
>> The fix is to detect structural changes made by the mapping functions passed to compute() and friends, and to throw a ConcurrentModificationException.  This prevents possibly adding entries in the wrong hash bin.
>>
>> I have updated the docs based on the prior discussion [1], making use of the new @implSpec/etc tags, as seemed sensible to me.  Suggestions welcome.
>>
>>
>> Automated test run is clean.  If the new docs look good, I'll start a CCC.
>>
>> Thanks,
>> -Brent
>>
>> 1. http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-February/031245.html
>>
>