Serguei, Thank you for the review.
New webrev is here: http://cr.openjdk.java.net/~dsamersoff/JDK-8059038/webrev.05/ I didn't change naming convention in SAGetoptTest.java and keep a_opt, b_opt etc as it gives better readability. Other concerns are addressed. BasicLauncherTest changed to use LingeredApp from testlib. -Dmitry On 2015-06-24 08:32, serguei.spit...@oracle.com wrote: > Hi Dmitry, > > Some quick minor comments. > > hotspot_webrev/agent/src/share/classes/sun/jvm/hotspot/SAGetopt.java > > Formatting is wrong: > > 57 if (_optind >_argv.length) { > > 71 String[] ca = carg.split("=",2); > > 80 if (los.contains(ca[0]+"=")) { > > > Need to use camel case for java method names: > > 55 private void extract_optarg(String opt) { > > 69 private String process_long_options(String carg, String[] longOptStr) { > > > Need to use quotes for '-': > > 109 // End of option batch like -abc reached, expect option to > start from - > > Example is: > > 133 // At this point carg[0] contains '-' > > > Wrong indent at 87, 139, 120-121: > > 85 else { > 86 // Mixed style options --file name > 87 extract_optarg(ca[0]); > 88 } > > 138 else { > 139 ch = carg.charAt(_optopt); > 140 } > > 119 if (longOptStr == null || longOptStr.length == 0) { > 120 // No long options specified, stop options processing > 121 return null; > 122 } > > > > hotspot_webrev/agent/src/share/classes/sun/jvm/hotspot/SALauncher.java > > Uninitialized local: > > 128 String s; > > Need to use camel case: > > 126 String exe_or_pid = null; > > > The main method is too long, I'd suggest to split with the sub-methods for: > clhsdb, hsdb, jstack, jmap, jinfo > > > jdk_webrev/test/sun/tools/jhsdb/BasicLauncherTest.java > > Wrong indent at 82, 85: > > 80 return toolProcess.exitValue(); > 81 } finally { > 82 theApp.stopApp(); > 83 } > 84 } catch (IOException | InterruptedException ex) { > 85 throw new RuntimeException("Test ERROR " + ex, ex); > 86 } > > > I do not understand what is the need for nested try statements, just one > try would be enough: > > 54 System.out.println("Starting LingeredApp"); > 55 try { > 56 try { > . . . > 81 } finally { > 82 theApp.stopApp(); > 83 } > 84 } catch (IOException | InterruptedException ex) { > 85 throw new RuntimeException("Test ERROR " + ex, ex); > 86 } > > 98 try { > 99 try { > . . . > 116 } finally { > 117 theApp.stopApp(); > 118 } > 119 } catch (Exception ex) { > 120 throw new RuntimeException("Test ERROR " + ex, ex); > 121 } > > > Why do you catch exceptions and throw the RuntimeException's in the > launch() methods > but catch the IOException in main? Would it be better to catch any > Exception? > > Too many empty lines: > > 88 > 89 > 90 > > > jdk_webrev/test/sun/tools/jhsdb/LingeredApp.java > > Too many empty lines: > > 275 > 276 > > 369 > > > jdk_webrev/test/sun/tools/jhsdb/SAGetoptTest.java > > Need to use Java naming convention: > > 36 private static boolean a_opt; > 37 private static boolean b_opt; > 38 private static boolean c_opt; > 39 private static boolean e_opt; > 40 private static boolean mixed_opt; > 41 > 42 private static String d_value; > 43 private static String exe_value; > 44 private static String core_value; > > Wrong indent 2 instead of 4: > > 70 if (s.equals("a")) { > 71 a_opt = true; > 72 continue; > 73 } > 74 > 75 if (s.equals("b")) { > 76 b_opt = true; > 77 continue; > 78 } > 79 > 80 if (s.equals("c")) { > 81 c_opt = true; > 82 continue; > 83 } > 84 > 85 if (s.equals("e")) { > 86 e_opt = true; > 87 continue; > 88 } > 89 > 90 if (s.equals("mixed")) { > 91 mixed_opt = true; > 92 continue; > 93 } > > > Thanks, > Serguei > > > On 6/23/15 7:06 AM, Dmitry Samersoff wrote: >> Hi Everybody, >> >> Please review the changes: >> >> http://cr.openjdk.java.net/~dsamersoff/JDK-8059038/webrev.04/ >> >> I'm about to file CCC request for it but would like to get internal >> feedback before that. >> >> This fix is introducing native launcher jhsdb for serviceability agent. >> >> jhsdb >> >> will launch command line debugger clhsdb >> >> jhsdb jstack file core >> jhsdb jmap file core >> jhsdb jinfo file core >> >> will launch corresponding SA based utility. >> >> >> -Dmitry >> >> > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
