Hi Kim,
On 12/3/2015 5:29 PM, Kim Barrett wrote:
On Dec 3, 2015, at 4:19 PM, Roger Riggs <roger.ri...@oracle.com> wrote:
...
src/java.base/share/classes/jdk/internal/misc/CleanerImpl.java
237 /**
238 * Returns true if the list's next reference refers to itself.
239 *
240 * @return true if the list is empty
241 */
242 boolean isListEmpty() {
243 PhantomCleanable<?> list = cleanerImpl.phantomCleanableList;
244 synchronized (list) {
245 return next == list;
246 }
247 }
Based on the description, one might expect some occurrence of "this"
in the body. This function must only be applied to the special head
sentinal, so a pre-condition is "list == this". Either an assert or
some mention of this would make this easier to understand.
[Applies to all three of {Soft,Weak,Phantom}Cleanable.]
The implicit reference to 'this' is in via cleanerImpl.
All of the functions insert, remove, isListEmpty need to lock on the list root
and explicitly referring to the list root (via the clearerImpl) in each method
keeps them aligned.
IsListEmpty could be recoded to operate only on the distinguished root entry
but it would be harder to see that the same lock is being used consistently.
I think that, as written, it would return true if applied to the last element
of a
non-empty list. (That never happens at present.) If the test were
"list == list.next” that (potential) failure mode would be eliminated. That
would also make the code and the comment line up nicely, though the
comment would then be kind of redundant, as it doesn’t provide any
explanation for why that works. Your call.
Good point, that would reduce some possible doubt. Will fix.
Thanks, Roger
So looks good to me.
