Review Request: Warnings cleanup in sun.management and its subpackages

[Stuart Marks]

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<MemoryManagerMXBean>, 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
>
>