> On Oct 20, 2015, at 8:44 AM, Daniel Fuchs <daniel.fu...@oracle.com> wrote: > > Hi Mandy, > > All feedback integrated - except those discussed below.
Thanks. > > public URL content has been refreshed: > http://cr.openjdk.java.net/~dfuchs/8046565/proto.02/webrev/index.html > http://cr.openjdk.java.net/~dfuchs/8046565/proto.02/specdiff-api/overview-summary.html > > sandbox branch JDK-8046565-branch updated as well. > : > >> sun/management/ManagementFactoryHelper.java >> >> 172 public interface LoggingMXBean >> 173 extends PlatformLoggingMXBean, java.util.logging.LoggingMXBean { >> 174 } > > Was there a comment related to that? This is a thorn that will prevent > java.management from not requiring java.logging. > Sorry I missed to go back and include my comment. The main thing I wanted to point out is that java.management will always require java.logging because the above interface. Just a note to you that the Class.forName call to find if java.util.logging.Logging class is present isn’t strictly necessary. It’s okay to keep it there to prepare if such dependency can be eliminated. I have filed JDK-8139982 to follow-up this dependency (this is unrelated to your change) > >> >> LazyLoggers.java >> 68 this(Objects.requireNonNull(loggerSupplier), >> 69 (Void)null); >> >> 130 this(Objects.requireNonNull(name), >> Objects.requireNonNull(factories), >> 131 Objects.requireNonNull(caller), null); >> >> I would expect Objects.requireNonNull on the constructor initializes all the >> fields. But the above lines checks non-null before calling this(….). Does >> the other constructor allow null parameters? Why not moving >> Objects.requireNonNull to the other constructor? > > Just a way to throw the exception before the super constructor > is called. > The other constructor is private. When we reach it the check > will already have been made. We could check again but for what purpose? My suggestion was to do the check for non-null in the other constructor (not to duplicate the check). Since they are private, it’s okay. > > >> java.util.logging.LogManager.demandLoggerFor >> - I think this method is intended for the LoggerFinder provider to use. >> Perhaps we should add a note in the javadoc to say that. > > Would you have something to suggest? > I was thinking that this method could also provide a > solution for JDK-8023163. As we discussed offline and you already made the change, this is not needed to be public for this JEP. Thanks Mandy
