Thanks for the review, my comments are inlined below.

On 01/ 5/12 05:19 PM, Karen Kinnear wrote:
Frederic,

Code looks good.

A couple of minor comments/questions:
1) diagnosticCommand.cpp HeapDumpDCmd::execute
    - There is a comment in attachListener under dump_heap that would
       be helpful to have here as well: // Request a full GC before heap dump 
if live_objects ...

A comment has been added in diagnosticCommand.cpp and
the help message of the HeapDumpDCmd has been updated too.

2) I wonder if it would be helpful for the  new jcmds that share functionality 
with existing mechanisms
     if it is worth a comment in the code noting where else this logic is 
called - might
     help future bug fixers do thorough testing.

I've added cross-references in comments between the attachListener
file and the diagnosticCommand file.

3) attachListener jcmd
    You've added an out->cr() after throwing the exception here, but I wonder if
    you actually want it whether or not there was an exception thrown here -
    e.g. inside execute I see a number of places where there are print calls for
    exceptions and raw_print called - do those want the<cr>  as well?
    Or are there cases where there is nothing printed so you would not want 
that? e.g. runFinalization?

The out->cr() in attachListener was correct, but other locations were
an exception was printed were lacking an out->cr() call. It's fixed now.

New webrev: http://cr.openjdk.java.net/~fparain/7120511/webrev.01/

Thank you,

Fred

--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com

Reply via email to