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

[Yasumasa Suenaga]

Thanks Serguei!

I will fix it and will push.


Yasumasa


On 2019/08/19 13:50, serguei.spitsyn at oracle.com wrote:
> 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.spitsyn at 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.spitsyn at 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
>>>>>
>>>
>