Re: PING: RFR: 8226204: SA: Refactoring for option processing in SALauncher

[serguei.spit...@oracle.com]

Hi Yasumasa, Thank you for the update! The typo below still is not fixed (replace: "be map" => "be mapped"): 225 * You also can set the options which cannot be map to old fashioned 242 * SAGetopt parses and validates the argument. If he user passes invalid 243 * option, SAGetoptException will be occurred at SAGetopt::next. 244 * Thus we need not to validate them in here.  A typo: "he user" => "the user".  I'd suggest to replace the line 244 with:    "Thus there is no need to validate it here." Thumbs up on the webrev in general. No need for re-review if you fix the above. Thanks, Serguei On 8/15/19 18:05, Yasumasa Suenaga wrote: Hi Serguei, Thank you for the comment. I fixed / added the comment in new webrev. Could you check again?   http://cr.openjdk.java.net/~ysuenaga/JDK-8226204/webrev.02/ Yasumasa On 2019/08/16 8:03, serguei.spit...@oracle.com wrote: Hi Yasumasa, Thank you for the update! A couple of suggestions: 213 * This method converts jhsdb-style options (oldArgs) to oldfashioned    Replace: "oldfashioned " => "old fashioned".    There are several occurrences of it. 214 * style. SALauncher delegates the work to the entry point of each tools.    Replace: "each tools" => "each tool" 225 * You also can set the options which cannot be map to oldfashioned 226 * arguments. For example, `jhsdb jmap --binaryheap` cannot be map to    Replace: "cannot be map" => "cannot be mapped" 231 * This method returns the map which the key is oldfashioned option, 232 * the value is its value.    I'd suggest to say:     * This method returns the map of the old fashioned key/val pairs. This loop still needs to be commented: 242 while ((s = sg.next(null, longOpts)) != null) { 243 var val = longOptsMap.get(s); 244 if (val != null) {                       // What is done here and why?   245 newArgMap.put(val, null); 246 } else { 247 val = longOptsMap.get(s + "=");  // Why the "=" is added 248 if (val != null) {                           // What is done here and why?   249 newArgMap.put(val, sg.getOptarg());   250                 }                       // Why there is no else statement, do we just skip the option?   251             }   252         }   Such comments will give more context and make this code more readable. Thanks, Serguei On 8/15/19 7:14 AM, Yasumasa Suenaga wrote: Hi Serguei, I added the explanation as a comment in parseOptions what is longOptsMap, and how parseOptions() work in new webrev. http://cr.openjdk.java.net/~ysuenaga/JDK-8226204/webrev.01/ I'm not good at English, so comments are welcome :) Thanks, Yasumasa On 2019/08/15 17:12, serguei.spit...@oracle.com wrote: Hi Yasumasa, In fact, I have a problem to understand what the parseOptions() method is doing. Could you add necessary comments explaining what is done in the loop? I'm sure I'll be not alone in having trouble to read this code. Also, it is not clear the approach with the longOptsMap's. Why do you need to map "exe=" to "exe" but "mixed" to "-m" and"clstats" to "-clstats"? It is better to be explained in the parseOptions() method as well. Thanks, Serguei On 8/10/19 04:14, Yasumasa Suenaga wrote: PING: Could you review it?    JBS: https://bugs.openjdk.java.net/browse/JDK-8226204    webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8226204/webrev.00/ Yasumasa On 2019/07/24 10:18, Yasumasa Suenaga wrote: Hi all, Please review this change:    JBS: https://bugs.openjdk.java.net/browse/JDK-8226204    webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8226204/webrev.00/ This enhancement has been proposed in [1]. SALauncher (jhsdb implementation) processes the option for each subcommand (e.g. jstack, hsdb). But they exist in many place with similar code. So there is some room for refactoring. This change has passed the tests on submit repo and serviceability/sa tests. Thanks, Yasumasa [1] https://mail.openjdk.java.net/pipermail/serviceability-dev/2019-June/028376.html