Serguei, Thank you for review.
> The following fragment has the same invariant for both branches: fixed. (webrev updated in-place, diff to webrev.06.old is attached) > One more suggestion would be to refactor the if (_optreset) { ... } > with a cal to a new method optReset(). I would prefer to leave it as is. The parser reaches one of *tree* sates after optReset: {end_of_processing, processLongOptions_result, need_to_process_short_options} so refactoring it to a separate method adds extra complication. -Dmitry On 2015-07-17 17:29, serguei.spit...@oracle.com wrote: > Dmitry, > > Thanks for new webrev! > > A couple of comments on > hotspot_webrev/agent/src/share/classes/sun/jvm/hotspot/SAGetopt.java > > The following fragment has the same invariant for both branches: > > 136 ch = carg.charAt(_optopt); > 137 } > 138 else { > 139 ch = carg.charAt(_optopt); > 140 } > > It can be replaced with: > > } > ch = carg.charAt(_optopt); > > > One more suggestion would be to refactor the if (_optreset) { ... } with a > cal to a new method optReset(). > > But I leave it up to you. > Thumbs up from me. > > > Thanks, > Serguei > > > On 7/17/15 7:13 AM, Dmitry Samersoff wrote: >> 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.
diff -ur webrevs_003/hotspot_webrev/raw_files/new/agent/src/share/classes/sun/jvm/hotspot/SAGetopt.java webrevs/hotspot_webrev/raw_files/new/agent/src/share/classes/sun/jvm/hotspot/SAGetopt.java --- webrevs_003/hotspot_webrev/raw_files/new/agent/src/share/classes/sun/jvm/hotspot/SAGetopt.java 2015-07-17 16:01:36.000000000 +0300 +++ webrevs/hotspot_webrev/raw_files/new/agent/src/share/classes/sun/jvm/hotspot/SAGetopt.java 2015-07-17 17:56:00.000000000 +0300 @@ -101,7 +101,6 @@ } String carg = _argv[_optind]; - char ch = '-'; _optarg = null; if (_optreset) { @@ -133,12 +132,10 @@ // At this point carg[0] contains '-' _optreset = false; _optopt = 1; - ch = carg.charAt(_optopt); - } - else { - ch = carg.charAt(_optopt); } + char ch = carg.charAt(_optopt); + // adjust pointer to next character _optopt += 1;