Review Request: Warnings cleanup in sun.management and its subpackages (original) (raw)

Stuart Marks stuart.marks at oracle.com
Mon Dec 5 16:49:52 PST 2011

Hi Mandy,

Thanks for the thorough review.

For warnings cleanup, we've been trying to avoid including refactorings like changing Enumerations to Iterator or to the enhanced-for loop. Changes to use try-with-resources also fall into this category. But I've been OK with such changes if the primary maintainer of the area is OK with them. Are you comfortable giving the go-ahead for these kinds of changes? I assume you are, since you suggested them, but I just wanted to double check.

Also, would any additional testing be required before these changes go in?

s'marks

On 12/5/11 4:28 PM, Mandy Chung wrote:

On 12/2/11 3:09 PM, Kurchi Hazra wrote:

Hi,

http://cr.openjdk.java.net/~khazra/7117570/webrev.00/ All these classes are implementation classes. These classes were implemented before the 1.5 language features were integrated. Below I included the suggestion to replace iterator with foreach since you are already in that file. Agent.java L221, 229: Since you're in that file, can you change it to call the UnsupportedOperationException(String msg, Throwable cause) constructor so that the cause exception is available? L252-271: it would be good to replace this try-finally with try-with-resource. ConnectorAddressLink.java L120-128: it'd be better to change this to use foreach. L172-173: the variable c in the for-loop should be of type Counter. GarbageCollectorImpl.java L76: another good candidate to use foreach. HotspotCompilation.java L139-144: another good candidate to use foreach. LazyCompositeData.java L162: for (String item : allItems) {...} MappedMXBeanType.java L76, 88: the cast should be fixed to "(Class<>)" L229, 759: I think these suppressed warnings can be eliminated. I have to study it a little bit to determine what we can do with this file and get back to you. NotificationEmitterSupport.java This class was copied fromjavax.management.NotificationBroadcasterSupport when it was implemented but the jmx class has been updated since then. It didn't use javax.management.NotificationBroadcasterSupport directly to avoid loading the jmx classes if java.lang.management is used locally with no jmx agent created. I think it's time to update this class and I'll submit a CR for that. Your change is fine. ConnectorBootstrap.java L237: nit: this extra line which can be removed. AdaptorBootstrap.java L130, 133: The com.sun.jmx.snmp API is not generified (it's internal API). Although the javadoc states that the getTrapDestinations method returns an enumeration of InetAddress, I think it'd be better for this change to wait until the com.sun.jmx.snmp API is updated. JvmMemManagerTableMetaImpl.java L70: should be indented (looks like it was accidentially removed in your change). L122: the returned type should be List, right? L107: JvmContextFactory.getUserData() returns a Map<Object,Object> which is the current implementation. This strikes me that this may lead to some type casting somewhere. e.g. JvmMemoryImpl.java L160 - this casts the key to MemoryUsage. Shouldn't javac issue unchecked warning for this? I wonder if you see any other warnings. I didn't review the rest of the snmp files (up to the JvmMemoryImpl.java file). It takes longer that I expect. It was very old code and looks like that it requires more investigation. I suggest to separate the snmp change and first get the non-snmp files warning cleaned and continue with the snmp one next. Thanks Mandy

More information about the jdk8-dev mailing list