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

[serguei.spitsyn at oracle.com]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190815/2c4b2fb6/attachment.html>