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

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Yasumasa Suenaga-4
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
Reply | Threaded
Open this post in threaded view
|

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

Yasumasa Suenaga-4
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
Reply | Threaded
Open this post in threaded view
|

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

Chris Plummer
Hi Yasumasa,

The changes look ok to me, although I've got to admit the language and
library features used by toolMap are a bit beyond what I'm comfortable
with (I'm one of those that find many uses of newer language and library
feature to be more of a hindrance to understanding code than they are a
benefit to simplifying or streamlining code). But I'm ok with it and
assume it works as the reader would expect (after staring at it for a bit).

I likely won't be able to do any re-review if more changes are needed
since I'll be out of the office for a while. I think Serguei is going to
do the 2nd review, so assuming he's ok with it, and any additional
changes are minor, you can still count me as a reviewer.

thanks,

Chris

On 8/10/19 4:14 AM, 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


Reply | Threaded
Open this post in threaded view
|

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

Yasumasa Suenaga-4
Thanks Chris!
I'm waiting for Serguei's review.


Yasumasa


On 2019/08/14 7:47, Chris Plummer wrote:

> Hi Yasumasa,
>
> The changes look ok to me, although I've got to admit the language and library features used by toolMap are a bit beyond what I'm comfortable with (I'm one of those that find many uses of newer language and library feature to be more of a hindrance to understanding code than they are a benefit to simplifying or streamlining code). But I'm ok with it and assume it works as the reader would expect (after staring at it for a bit).
>
> I likely won't be able to do any re-review if more changes are needed since I'll be out of the office for a while. I think Serguei is going to do the 2nd review, so assuming he's ok with it, and any additional changes are minor, you can still count me as a reviewer.
>
> thanks,
>
> Chris
>
> On 8/10/19 4:14 AM, 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
>
>
Reply | Threaded
Open this post in threaded view
|

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

serguei.spitsyn@oracle.com
In reply to this post by Yasumasa Suenaga-4
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

Reply | Threaded
Open this post in threaded view
|

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

Yasumasa Suenaga-4
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, [hidden email] 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
>
Reply | Threaded
Open this post in threaded view
|

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

serguei.spitsyn@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, [hidden email] 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


Reply | Threaded
Open this post in threaded view
|

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

Yasumasa Suenaga-4
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, [hidden email] 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, [hidden email] 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
>>>
>
Reply | Threaded
Open this post in threaded view
|

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

serguei.spitsyn@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, [hidden email] 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, [hidden email] 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



Reply | Threaded
Open this post in threaded view
|

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

Yasumasa Suenaga-4
Thanks Serguei!

I will fix it and will push.


Yasumasa


On 2019/08/19 13:50, [hidden email] 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, [hidden email] 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, [hidden email] 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
>>>>>
>>>
>