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

[Yasumasa Suenaga]

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
>>>
>