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

On 20 Mar 2015, at 09:52, Paul Sandoz <paul.sandoz@oracle.com> wrote:

On Mar 19, 2015, at 11:31 PM, Brent Christian <brent.christian@oracle.com> wrote:

...

Hi, Paul

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

Looking good.

Yes, the spec changes look good. Brent, would you mind if I assign 8062841 [1] to you, so you can bring in the CHM implementation changes? Also maybe CHM could be added to the test?

Small point. We could clear up some inconsistency over using "the function" or the "the re/mapping function" to constantly use the later and call it that in the doc of the "re/mappingFunction" parameter.

...

TestNG test update forthcoming.

Ok.

-Chris. [1] https://bugs.openjdk.java.net/browse/JDK-8062841

Here's the latest: http://cr.openjdk.java.net/~bchristi/8071667/webrev.4/ That should have everything. Thanks again, -Brent On 3/20/15 2:52 AM, Paul Sandoz wrote:

On Mar 19, 2015, at 11:31 PM, Brent Christian <brent.christian@oracle.com> wrote:

...

Hi, Paul

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

Looking good.

Small point. We could clear up some inconsistency over using "the function" or the "the re/mapping function" to constantly use the later and call it that in the doc of the "re/mappingFunction" parameter.

...

TestNG test update forthcoming.

Ok.

Paul.

I wonder if we, optionally, pass the exception type, either CME or IAE, could we add CHM to the DataProvider? On 21/03/15 13:43, Paul Sandoz wrote:

On Mar 20, 2015, at 10:23 PM, Brent Christian <brent.christian@oracle.com> wrote:

...

Here's the latest:

http://cr.openjdk.java.net/~bchristi/8071667/webrev.4/

That should have everything.

Looks good (i assumed all but the test code has remained unchanged).

I like the test. A minor comment:

125 private static void checkCME(Consumer code, boolean expectCME) { 126 try { 127 code.accept(null); 128 } catch (ConcurrentModificationException cme) { 129 if (expectCME) { return; } else { throw cme; } 130 } 131 if (expectCME) { 132 throw new RuntimeException("Expected CME, but wasn't thrown"); 133 } 134 } 135 136 private static BiFunction<String,String,String> mkBiFunc(Map map) { 137 return (k,v) -> { 138 putToForceRehash(map); 139 return "computedValue"; 140 }; 141 }

Can you avoid using raw types?

Yes, all test code should be lint free ;-) Or at least all new test code ( where practicable ). -Chris.

Paul.

On 23/03/15 11:35, Paul Sandoz wrote:

On Mar 23, 2015, at 12:16 PM, Chris Hegarty <chris.hegarty@oracle.com> wrote:

...

I wonder if we, optionally, pass the exception type, either CME or IAE, could we add CHM to the DataProvider?

Possibly, but I suspect any tests for CHM will be more fragile and involved. My sense is it may be premature to do that right now.

OK, no problem. Just a thought.

To tickle the locations where ISEs are thrown the CHM compute functions probably need to perturb the bucket where the key computed on is, or is to be, located and tests might need to perform concurrent updates to induce resizes.

-Chris.

Paul.