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