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