Serguei, Sorry for typeo
new webrev: http://cr.openjdk.java.net/~dsamersoff/JDK-8059038/webrev.06 -Dmitry On 2015-07-17 16:28, serguei.spit...@oracle.com wrote: > On 7/17/15 6:21 AM, Dmitry Samersoff wrote: >> Serguei, >> >> new webrev: >> >> http://cr.openjdk.java.net/~dsamersoff/JDK-8059038/webrev.05 > > The webrev is the same. > I do not see the changes you claim below. > Could you, please, generate a webrev with another version number? > > > Thanks, > Serguei > >> >> diff to webrev.05.old attached >> >> please see also below. >> >> On 2015-07-17 13:46, serguei.spit...@oracle.com wrote: >>> Dmitry, >>> >>> Thanks for the diff, it helps! >>> >>> hotspot_webrev/agent/src/share/classes/sun/jvm/hotspot/SAGetopt.java >>> >>> Uninitialized local definition: >>> >>> 105 char ch; >> changed. ch is initialized later, so we actually don't need it. >> >> >>> Unneded second initialization at 111: >>> 104 String carg = _argv[_optind]; >>> 111 carg = _argv[_optind]; >> fixed. >> >>> It is not clear why carg can't be empty: >>> >>> 61 // _argv[_optind] can't be empty, so it's safe to >>> expect at least one character >>> 62 if (_argv[_optind].charAt(0) == '-') { >>> ... >>> >>> 113 // carg can't be empty so it's safe to expect at >>> least one character >>> 114 if (carg.charAt(0) != '-' || carg.equals("--")) { >> changed. >> >> An array passed to getopt is result of splitting arguments string, so no >> empty array element possible. But changed it to be on safe side. >> >>> The _argv comes from outside of the method. >>> How can we be sure that the value _argv[_optind] is not empty String? >>> Does it comes from an assumption that the outside processing works >>> correctly? >>> Would it be better to always check that it is not empty? >>> >>> >>> It feels like this code is not clear and more complex than has to be. >>> But I can't tell yet what has to be simplified. >>> >>> For example, I do not like this part: >>> 37 private boolean _optreset; // special handling of first call >>> >>> 44 _optreset = true; >>> >>> 108 if (_optreset) { >>> >>> 138 _optreset = false; >>> >>> >>> Would it be better to separate this first step from the next() method >>> and make it a separate method that is called reset() or init()? >> Reset called every time when we finish the option batch: >> >> prog -xzvf filename /*reset here*/ -abc >> >>> Also, there is no clue why all this is necessary. >> This is a port of standard BSD getopt (based on C++ code I wrote back in >> 2004), that takes care of all possible option combinations and allow to >> process it uniform way. >> >> I would love to have it JDK-wide instead of a separate parser for each >> tool. >> >>> Other files look good to me. >>> Do you have another reviewer? >> Stefan Larsen reviewed one of the previous versions. >> >> -Dmitry >> >>> >>> >>> On 7/17/15 2:46 AM, Dmitry Samersoff wrote: >>>> Serguei, >>>> >>>> Previous webrev is: >>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8059038/webrev.05.old >>>> >>>> Latest webrev is: >>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8059038/webrev.05 >>>> >>>> Diff between webrev.05.old and webrev.05 attached >>>> >>>> -Dmitry >>>> >>>> On 2015-07-17 01:00, serguei.spit...@oracle.com wrote: >>>>> Hi Dmitry, >>>>> >>>>> I do not see any changes. >>>>> Could you please, generate .06 version ? >>>>> In such a case, it will be much easier to compare the code. >>>>> >>>>> Thanks, >>>>> Serguei >>>>> >>>>> On 7/16/15 8:23 AM, Dmitry Samersoff wrote: >>>>>> Serguei, >>>>>> >>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8059038/webrev.05 >>>>>> >>>>>> Webrev updated in-place (press shift-reload). >>>>>> >>>>>> Code changes at ll.119. >>>>>> >>>>>> Added more comments to other places. >>>>>> >>>>>> -Dmitry >>>>>> >>>>>> On 2015-06-27 03:15, serguei.spit...@oracle.com wrote: >>>>>>> Hi Dmitry, >>>>>>> >>>>>>> Thank you for the update! >>>>>>> The SALauncher.java changes are really nice. >>>>>>> I have just couple of small comments. >>>>>>> >>>>>>> agent/src/share/classes/sun/jvm/hotspot/SALauncher.java >>>>>>> >>>>>>> 343 // Run tmtools >>>>>>> 344 if (args[0].equals("jstack")) { >>>>>>> 345 runJSTACK(oldArgs); >>>>>>> >>>>>>> Why the comment says "Run tmtools", not jstack? >>>>>>> BTW, other fragments have no such a comment which is Ok at it is >>>>>>> obvious. >>>>>>> >>>>>>> >>>>>>> agent/src/share/classes/sun/jvm/hotspot/SAGetopt.java >>>>>>> >>>>>>> There are no checks of the carg length in several places where it is >>>>>>> needed: >>>>>>> >>>>>>> 61 if (_argv[_optind].charAt(0) == '-') { >>>>>>> >>>>>>> 112 if (carg.charAt(0) != '-' || carg.equals("--")) { >>>>>>> 117 if (carg.charAt(0) == '-' && carg.charAt(1) == >>>>>>> '-') { >>>>>>> 124 carg = carg.substring(2); >>>>>>> >>>>>>> 136 ch = carg.charAt(_optopt); >>>>>>> 139 ch = carg.charAt(_optopt); >>>>>>> >>>>>>> >>>>>>> Otherwise, the fix looks good. >>>>>>> >>>>>>> >>>>>>> Thanks, >>>>>>> Serguei >>>>>>> >>>>>>> >>>>>>> On 6/24/15 5:37 AM, Dmitry Samersoff wrote: >>>>>>>> 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.
