Thanks Serguei!
I will fix it and will push.
Yasumasa
On 2019/08/19 13:50, [email protected] 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, [email protected] 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, [email protected] 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
