Hi Mandy,
The webrev[1] and javadoc[2] are updated in to address your comments.
Thanks, Roger
[1] http://cr.openjdk.java.net/~rriggs/webrev-cleaner-8138696/
[2] http://cr.openjdk.java.net/~rriggs/cleaner-doc/index.html
On 12/3/2015 8:43 PM, Mandy Chung wrote:
On Dec 3, 2015, at 1:19 PM, Roger Riggs <roger.ri...@oracle.com> wrote:
[1] http://cr.openjdk.java.net/~rriggs/cleaner-doc/
[2] http://cr.openjdk.java.net/~rriggs/webrev-cleaner-8138696/
The implementation looks good. I’ll wait for an updated javadoc and re-review
that incorporates the editorial comments that Mark made.
Some additional comments:
- null should be {@code null}
150 * Refer to the API Note above for cautions about the behavior
151 * of cleaning functions.
ok
It may be good to add a link back to the API note.
ok
76 * and not block. If the cleaning function blocks it may delay processing
- should there be “,” before “it…”
ok
77 * other cleaning functions using the same Cleaner.
78 * All cleaning functions using a Cleaner should be mutually compatible.
- “using a cleaner” - should the terminology be “registered in a cleaner”
consistent with other references in the spec?
ok
104 * The thread sets the ThreadContextClassLoader to the system class
loader,
105 * sets the AccessControlContext to an empty ProtectionDomain,
106 * clears ThreadLocals, and sets the thread to be a
107 * {@link Thread#setDaemon(boolean) daemon thread}.
I think clearing thread locals may not need to be mentioned. Below is one
suggestion to rephrase the above. Similarly can be applied to the thread
factory variant.
* The thread is a {@linkplain Thread#isDaemon() daemon thread}.
* The thread's {@linkplain Thread#getContextClassLoader context class loader}
* is set to {@linkplain ClassLoader#getSystemClassLoader() system class loader}.
* The thread has no permission to access any security operation.
Reworded both method doc.
Minor comment on the CleanerTest.java:
530 // unless it is know this case throws an exception,
rethrow
typo: s/know/known
It would be better to rename the test[1-5] methods to reflect what it verifies.
done
254 Reference<?> r = queue.remove();
Minor one: remove(long timeout) may be a better option while in practice
remove() will not block forever.
done.
Roger
Mandy
