Skip to content

Commit

Permalink
simplify some comments
Browse files Browse the repository at this point in the history
  • Loading branch information
bchristi-git committed Jul 12, 2024
1 parent 4fc8b6d commit 1983bbc
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 32 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ public void run() {
private final Cleanable cleanable;

// Subclasses interact directly with the LdapCtx. This method provides
// access to the LdapCtx in the EnumCtx.
// access to the LdapCtx within the EnumCtx.
protected final LdapCtx getHomeCtx() { return enumCtx.homeCtx; }

/*
Expand Down Expand Up @@ -164,12 +164,9 @@ public final T nextElement() {
try {
return next();
} finally {
// An interesting one.

// A similar case to nextImpl(), except in this case, next()
// *is* overridable. Add fences here; otherwise, next() could
// be incorrectly overridden in the future to access the
// cleanable state without the proper fences.
// See comment in nextImpl(). This is similar, but in this case, next()
// *is* overridable. Fences are included here, in case next() is
// overridden to access the cleanable state without the proper fences.

// Ensure writes are visible to the Cleaner thread
VarHandle.fullFence();
Expand All @@ -179,24 +176,6 @@ public final T nextElement() {
} catch (NamingException e) {
// can't throw exception
cleanup();
// Re: automagically adding fences someday...
// Methods which do explicit cleanup() like this are...interesting. If
// fences are added after *every* method call, in this case the
// fences would "happen" after cleanup had already happened.
//
// Is there a way for such an automatically-added reachabilityFence
// to cause problems? Could the method require some object to
// became unreachable? I don't think so: the reachabilityFence keeps
// 'this' from becoming unreachable, and a method could not reset the
// value of 'this', e.g. to null, to cause it to become unreachable.
//
// In the cases like this of an explicit call to cleanup(),
// the cleanup happens on the main/program thread, not on the cleaner
// thread. So there's no visibility issues with the cleaner thread
// (but perhaps still could be with some other thread?).
//
// So automatic reachability and full fences in this would be strange,
// but not harmful AFAICT.
return null;
}
}
Expand All @@ -207,7 +186,7 @@ public final boolean hasMoreElements() {
try {
return hasMore();
} finally {
// See nextElement(), above - same situation
// Same situation as nextElement() - see comment above

// Ensure writes are visible to the Cleaner thread
VarHandle.fullFence();
Expand Down Expand Up @@ -375,9 +354,8 @@ private T nextImpl() throws NamingException {
cleanup();
throw cont.fillInException(e);
}
// No fences, but another interesting one.
// nextAux() (source just below) has its own fences. The only other thing
// this method does is to *call cleanup*.
// No fences here. nextAux() (source just below) has its own fences. The
// only other thing this method does is call cleanup().
// If nextAux() were overrideable, this method should probably have
// fences, but it seems OK for now without.
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ final class LdapSearchEnumeration
// fully qualified name of starting context of search
startName = new LdapName(starter);
searchArgs = args;
// No fences here?
// No fences here for now.
// super() call (aka AbstrctLdapNamingEnumeration ctor) keeps 'this'
// reachable until end of Cleaner registration. Code after that
// clearly does not touch cleanable state.
Expand Down Expand Up @@ -208,7 +208,7 @@ public void appendUnprocessedReferrals(LdapReferralException ex) {
// a referral has been followed so do not create relative names
startName = null;
super.appendUnprocessedReferrals(ex);
// No fences here?
// No fences here for now.
// Rely on fences in the super call.
// Other code in this method clearly doesn't access cleanable state.
}
Expand All @@ -235,7 +235,7 @@ protected void update(AbstractLdapNamingEnumeration<? extends NameClassPair> ne)
// Update search-specific variables
LdapSearchEnumeration se = (LdapSearchEnumeration)ne;
startName = se.startName;
// No fences here?
// No fences here for now.
// super.update() (aka AbstractLdapNamingEnumeration.update()) already
// keeps 'this' and 'ne' reachable throughout that method call.
// After that, we're clearly not accessing cleanable state from 'this'
Expand Down

0 comments on commit 1983bbc

Please sign in to comment.