Thank you for the feedback. Responses inline. On Oct 22 2013, at 18:45 , David Holmes <david.hol...@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.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 >>>> >>
