Serguei,
Thank you for review.
> The following fragment has the same invariant for both branches:
fixed. (webrev updated in-place, diff to webrev.06.old is attached)
> One more suggestion would be to refactor the if (_optreset) { ... }
> with a cal to a new method optReset().
I would prefer to leave it as is.
The parser reaches one of *tree* sates after optReset:
{end_of_processing, processLongOptions_result,
need_to_process_short_options}
so refactoring it to a separate method adds extra complication.
-Dmitry
On 2015-07-17 17:29, serguei.spit...@oracle.com wrote:
> Dmitry,
>
> Thanks for new webrev!
>
> A couple of comments on
> hotspot_webrev/agent/src/share/classes/sun/jvm/hotspot/SAGetopt.java
>
> The following fragment has the same invariant for both branches:
>
> 136 ch = carg.charAt(_optopt);
> 137 }
> 138 else {
> 139 ch = carg.charAt(_optopt);
> 140 }
>
> It can be replaced with:
>
> }
> ch = carg.charAt(_optopt);
>
>
> One more suggestion would be to refactor the if (_optreset) { ... } with a
> cal to a new method optReset().
>
> But I leave it up to you.
> Thumbs up from me.
>
>
> Thanks,
> Serguei
>
>
> On 7/17/15 7:13 AM, Dmitry Samersoff wrote:
>> Serguei,
>>
>> Sorry for typeo
>>
>> new webrev:
>> http://cr.openjdk.java.net/~dsamersoff/JDK-8059038/webrev.06
>>
>> -Dmitry
>>
>>
>> On 2015-07-17 16:28, serguei.spit...@oracle.com wrote:
>>> On 7/17/15 6:21 AM, Dmitry Samersoff wrote:
>>>> Serguei,
>>>>
>>>> new webrev:
>>>>
>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8059038/webrev.05
>>> The webrev is the same.
>>> I do not see the changes you claim below.
>>> Could you, please, generate a webrev with another version number?
>>>
>>>
>>> Thanks,
>>> Serguei
>>>
>>>> diff to webrev.05.old attached
>>>>
>>>> please see also below.
>>>>
>>>> On 2015-07-17 13:46, serguei.spit...@oracle.com wrote:
>>>>> Dmitry,
>>>>>
>>>>> Thanks for the diff, it helps!
>>>>>
>>>>> hotspot_webrev/agent/src/share/classes/sun/jvm/hotspot/SAGetopt.java
>>>>>
>>>>> Uninitialized local definition:
>>>>>
>>>>> 105 char ch;
>>>> changed. ch is initialized later, so we actually don't need it.
>>>>
>>>>
>>>>> Unneded second initialization at 111:
>>>>> 104 String carg = _argv[_optind];
>>>>> 111 carg = _argv[_optind];
>>>> fixed.
>>>>
>>>>> It is not clear why carg can't be empty:
>>>>>
>>>>> 61 // _argv[_optind] can't be empty, so it's safe to
>>>>> expect at least one character
>>>>> 62 if (_argv[_optind].charAt(0) == '-') {
>>>>> ...
>>>>>
>>>>> 113 // carg can't be empty so it's safe to expect at
>>>>> least one character
>>>>> 114 if (carg.charAt(0) != '-' || carg.equals("--")) {
>>>> changed.
>>>>
>>>> An array passed to getopt is result of splitting arguments string, so no
>>>> empty array element possible. But changed it to be on safe side.
>>>>
>>>>> The _argv comes from outside of the method.
>>>>> How can we be sure that the value _argv[_optind] is not empty String?
>>>>> Does it comes from an assumption that the outside processing works
>>>>> correctly?
>>>>> Would it be better to always check that it is not empty?
>>>>>
>>>>>
>>>>> It feels like this code is not clear and more complex than has to be.
>>>>> But I can't tell yet what has to be simplified.
>>>>>
>>>>> For example, I do not like this part:
>>>>> 37 private boolean _optreset; // special handling of first call
>>>>>
>>>>> 44 _optreset = true;
>>>>>
>>>>> 108 if (_optreset) {
>>>>>
>>>>> 138 _optreset = false;
>>>>>
>>>>>
>>>>> Would it be better to separate this first step from the next() method
>>>>> and make it a separate method that is called reset() or init()?
>>>> Reset called every time when we finish the option batch:
>>>>
>>>> prog -xzvf filename /*reset here*/ -abc
>>>>
>>>>> Also, there is no clue why all this is necessary.
>>>> This is a port of standard BSD getopt (based on C++ code I wrote back in
>>>> 2004), that takes care of all possible option combinations and allow to
>>>> process it uniform way.
>>>>
>>>> I would love to have it JDK-wide instead of a separate parser for each
>>>> tool.
>>>>
>>>>> Other files look good to me.
>>>>> Do you have another reviewer?
>>>> Stefan Larsen reviewed one of the previous versions.
>>>>
>>>> -Dmitry
>>>>
>>>>>
>>>>> On 7/17/15 2:46 AM, Dmitry Samersoff wrote:
>>>>>> Serguei,
>>>>>>
>>>>>> Previous webrev is:
>>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8059038/webrev.05.old
>>>>>>
>>>>>> Latest webrev is:
>>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8059038/webrev.05
>>>>>>
>>>>>> Diff between webrev.05.old and webrev.05 attached
>>>>>>
>>>>>> -Dmitry
>>>>>>
>>>>>> On 2015-07-17 01:00, serguei.spit...@oracle.com wrote:
>>>>>>> Hi Dmitry,
>>>>>>>
>>>>>>> I do not see any changes.
>>>>>>> Could you please, generate .06 version ?
>>>>>>> In such a case, it will be much easier to compare the code.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Serguei
>>>>>>>
>>>>>>> On 7/16/15 8:23 AM, Dmitry Samersoff wrote:
>>>>>>>> Serguei,
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8059038/webrev.05
>>>>>>>>
>>>>>>>> Webrev updated in-place (press shift-reload).
>>>>>>>>
>>>>>>>> Code changes at ll.119.
>>>>>>>>
>>>>>>>> Added more comments to other places.
>>>>>>>>
>>>>>>>> -Dmitry
>>>>>>>>
>>>>>>>> On 2015-06-27 03:15, serguei.spit...@oracle.com wrote:
>>>>>>>>> Hi Dmitry,
>>>>>>>>>
>>>>>>>>> Thank you for the update!
>>>>>>>>> The SALauncher.java changes are really nice.
>>>>>>>>> I have just couple of small comments.
>>>>>>>>>
>>>>>>>>> agent/src/share/classes/sun/jvm/hotspot/SALauncher.java
>>>>>>>>>
>>>>>>>>> 343 // Run tmtools
>>>>>>>>> 344 if (args[0].equals("jstack")) {
>>>>>>>>> 345 runJSTACK(oldArgs);
>>>>>>>>>
>>>>>>>>> Why the comment says "Run tmtools", not jstack?
>>>>>>>>> BTW, other fragments have no such a comment which is Ok at it is
>>>>>>>>> obvious.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> agent/src/share/classes/sun/jvm/hotspot/SAGetopt.java
>>>>>>>>>
>>>>>>>>> There are no checks of the carg length in several places where it is
>>>>>>>>> needed:
>>>>>>>>>
>>>>>>>>> 61 if (_argv[_optind].charAt(0) == '-') {
>>>>>>>>>
>>>>>>>>> 112 if (carg.charAt(0) != '-' || carg.equals("--")) {
>>>>>>>>> 117 if (carg.charAt(0) == '-' && carg.charAt(1) ==
>>>>>>>>> '-') {
>>>>>>>>> 124 carg = carg.substring(2);
>>>>>>>>>
>>>>>>>>> 136 ch = carg.charAt(_optopt);
>>>>>>>>> 139 ch = carg.charAt(_optopt);
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Otherwise, the fix looks good.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Serguei
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 6/24/15 5:37 AM, Dmitry Samersoff wrote:
>>>>>>>>>> Serguei,
>>>>>>>>>>
>>>>>>>>>> Thank you for the review.
>>>>>>>>>>
>>>>>>>>>> New webrev is here:
>>>>>>>>>>
>>>>>>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8059038/webrev.05/
>>>>>>>>>>
>>>>>>>>>> I didn't change naming convention in SAGetoptTest.java and keep
>>>>>>>>>> a_opt,
>>>>>>>>>> b_opt etc as it gives better readability. Other concerns are
>>>>>>>>>> addressed.
>>>>>>>>>>
>>>>>>>>>> BasicLauncherTest changed to use LingeredApp from testlib.
>>>>>>>>>>
>>>>>>>>>> -Dmitry
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 2015-06-24 08:32, serguei.spit...@oracle.com wrote:
>>>>>>>>>>> Hi Dmitry,
>>>>>>>>>>>
>>>>>>>>>>> Some quick minor comments.
>>>>>>>>>>>
>>>>>>>>>>> hotspot_webrev/agent/src/share/classes/sun/jvm/hotspot/SAGetopt.java
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Formatting is wrong:
>>>>>>>>>>>
>>>>>>>>>>> 57 if (_optind >_argv.length) {
>>>>>>>>>>>
>>>>>>>>>>> 71 String[] ca = carg.split("=",2);
>>>>>>>>>>>
>>>>>>>>>>> 80 if (los.contains(ca[0]+"=")) {
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Need to use camel case for java method names:
>>>>>>>>>>>
>>>>>>>>>>> 55 private void extract_optarg(String opt) {
>>>>>>>>>>>
>>>>>>>>>>> 69 private String process_long_options(String carg, String[]
>>>>>>>>>>> longOptStr) {
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Need to use quotes for '-':
>>>>>>>>>>>
>>>>>>>>>>> 109 // End of option batch like -abc reached, expect
>>>>>>>>>>> option to start from -
>>>>>>>>>>>
>>>>>>>>>>> Example is:
>>>>>>>>>>>
>>>>>>>>>>> 133 // At this point carg[0] contains '-'
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Wrong indent at 87, 139, 120-121:
>>>>>>>>>>>
>>>>>>>>>>> 85 else {
>>>>>>>>>>> 86 // Mixed style options --file name
>>>>>>>>>>> 87 extract_optarg(ca[0]);
>>>>>>>>>>> 88 }
>>>>>>>>>>>
>>>>>>>>>>> 138 else {
>>>>>>>>>>> 139 ch = carg.charAt(_optopt);
>>>>>>>>>>> 140 }
>>>>>>>>>>>
>>>>>>>>>>> 119 if (longOptStr == null ||
>>>>>>>>>>> longOptStr.length ==
>>>>>>>>>>> 0) {
>>>>>>>>>>> 120 // No long options specified, stop
>>>>>>>>>>> options
>>>>>>>>>>> processing
>>>>>>>>>>> 121 return null;
>>>>>>>>>>> 122 }
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> hotspot_webrev/agent/src/share/classes/sun/jvm/hotspot/SALauncher.java
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Uninitialized local:
>>>>>>>>>>>
>>>>>>>>>>> 128 String s;
>>>>>>>>>>>
>>>>>>>>>>> Need to use camel case:
>>>>>>>>>>>
>>>>>>>>>>> 126 String exe_or_pid = null;
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> The main method is too long, I'd suggest to split with the
>>>>>>>>>>> sub-methods for:
>>>>>>>>>>> clhsdb, hsdb, jstack, jmap, jinfo
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> jdk_webrev/test/sun/tools/jhsdb/BasicLauncherTest.java
>>>>>>>>>>>
>>>>>>>>>>> Wrong indent at 82, 85:
>>>>>>>>>>>
>>>>>>>>>>> 80 return toolProcess.exitValue();
>>>>>>>>>>> 81 } finally {
>>>>>>>>>>> 82 theApp.stopApp();
>>>>>>>>>>> 83 }
>>>>>>>>>>> 84 } catch (IOException | InterruptedException ex) {
>>>>>>>>>>> 85 throw new RuntimeException("Test ERROR "
>>>>>>>>>>> + ex,
>>>>>>>>>>> ex);
>>>>>>>>>>> 86 }
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I do not understand what is the need for nested try statements,
>>>>>>>>>>> just one
>>>>>>>>>>> try would be enough:
>>>>>>>>>>>
>>>>>>>>>>> 54 System.out.println("Starting LingeredApp");
>>>>>>>>>>> 55 try {
>>>>>>>>>>> 56 try {
>>>>>>>>>>> . . .
>>>>>>>>>>> 81 } finally {
>>>>>>>>>>> 82 theApp.stopApp();
>>>>>>>>>>> 83 }
>>>>>>>>>>> 84 } catch (IOException | InterruptedException ex) {
>>>>>>>>>>> 85 throw new RuntimeException("Test ERROR "
>>>>>>>>>>> + ex,
>>>>>>>>>>> ex);
>>>>>>>>>>> 86 }
>>>>>>>>>>>
>>>>>>>>>>> 98 try {
>>>>>>>>>>> 99 try {
>>>>>>>>>>> . . .
>>>>>>>>>>> 116 } finally {
>>>>>>>>>>> 117 theApp.stopApp();
>>>>>>>>>>> 118 }
>>>>>>>>>>> 119 } catch (Exception ex) {
>>>>>>>>>>> 120 throw new RuntimeException("Test ERROR " +
>>>>>>>>>>> ex, ex);
>>>>>>>>>>> 121 }
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Why do you catch exceptions and throw the RuntimeException's in
>>>>>>>>>>> the
>>>>>>>>>>> launch() methods
>>>>>>>>>>> but catch the IOException in main? Would it be better to catch any
>>>>>>>>>>> Exception?
>>>>>>>>>>>
>>>>>>>>>>> Too many empty lines:
>>>>>>>>>>>
>>>>>>>>>>> 88
>>>>>>>>>>> 89
>>>>>>>>>>> 90
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> jdk_webrev/test/sun/tools/jhsdb/LingeredApp.java
>>>>>>>>>>>
>>>>>>>>>>> Too many empty lines:
>>>>>>>>>>>
>>>>>>>>>>> 275
>>>>>>>>>>> 276
>>>>>>>>>>>
>>>>>>>>>>> 369
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> jdk_webrev/test/sun/tools/jhsdb/SAGetoptTest.java
>>>>>>>>>>>
>>>>>>>>>>> Need to use Java naming convention:
>>>>>>>>>>>
>>>>>>>>>>> 36 private static boolean a_opt;
>>>>>>>>>>> 37 private static boolean b_opt;
>>>>>>>>>>> 38 private static boolean c_opt;
>>>>>>>>>>> 39 private static boolean e_opt;
>>>>>>>>>>> 40 private static boolean mixed_opt;
>>>>>>>>>>> 41
>>>>>>>>>>> 42 private static String d_value;
>>>>>>>>>>> 43 private static String exe_value;
>>>>>>>>>>> 44 private static String core_value;
>>>>>>>>>>>
>>>>>>>>>>> Wrong indent 2 instead of 4:
>>>>>>>>>>>
>>>>>>>>>>> 70 if (s.equals("a")) {
>>>>>>>>>>> 71 a_opt = true;
>>>>>>>>>>> 72 continue;
>>>>>>>>>>> 73 }
>>>>>>>>>>> 74
>>>>>>>>>>> 75 if (s.equals("b")) {
>>>>>>>>>>> 76 b_opt = true;
>>>>>>>>>>> 77 continue;
>>>>>>>>>>> 78 }
>>>>>>>>>>> 79
>>>>>>>>>>> 80 if (s.equals("c")) {
>>>>>>>>>>> 81 c_opt = true;
>>>>>>>>>>> 82 continue;
>>>>>>>>>>> 83 }
>>>>>>>>>>> 84
>>>>>>>>>>> 85 if (s.equals("e")) {
>>>>>>>>>>> 86 e_opt = true;
>>>>>>>>>>> 87 continue;
>>>>>>>>>>> 88 }
>>>>>>>>>>> 89
>>>>>>>>>>> 90 if (s.equals("mixed")) {
>>>>>>>>>>> 91 mixed_opt = true;
>>>>>>>>>>> 92 continue;
>>>>>>>>>>> 93 }
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Serguei
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 6/23/15 7:06 AM, Dmitry Samersoff wrote:
>>>>>>>>>>>> Hi Everybody,
>>>>>>>>>>>>
>>>>>>>>>>>> Please review the changes:
>>>>>>>>>>>>
>>>>>>>>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8059038/webrev.04/
>>>>>>>>>>>>
>>>>>>>>>>>> I'm about to file CCC request for it but would like to get
>>>>>>>>>>>> internal
>>>>>>>>>>>> feedback before that.
>>>>>>>>>>>>
>>>>>>>>>>>> This fix is introducing native launcher jhsdb for serviceability
>>>>>>>>>>>> agent.
>>>>>>>>>>>>
>>>>>>>>>>>> jhsdb
>>>>>>>>>>>>
>>>>>>>>>>>> will launch command line debugger clhsdb
>>>>>>>>>>>>
>>>>>>>>>>>> jhsdb jstack file core
>>>>>>>>>>>> jhsdb jmap file core
>>>>>>>>>>>> jhsdb jinfo file core
>>>>>>>>>>>>
>>>>>>>>>>>> will launch corresponding SA based utility.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> -Dmitry
>>>>>>>>>>>>
>>>>>>>>>>>>
>>
>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
diff -ur
webrevs_003/hotspot_webrev/raw_files/new/agent/src/share/classes/sun/jvm/hotspot/SAGetopt.java
webrevs/hotspot_webrev/raw_files/new/agent/src/share/classes/sun/jvm/hotspot/SAGetopt.java
---
webrevs_003/hotspot_webrev/raw_files/new/agent/src/share/classes/sun/jvm/hotspot/SAGetopt.java
2015-07-17 16:01:36.000000000 +0300
+++
webrevs/hotspot_webrev/raw_files/new/agent/src/share/classes/sun/jvm/hotspot/SAGetopt.java
2015-07-17 17:56:00.000000000 +0300
@@ -101,7 +101,6 @@
}
String carg = _argv[_optind];
- char ch = '-';
_optarg = null;
if (_optreset) {
@@ -133,12 +132,10 @@
// At this point carg[0] contains '-'
_optreset = false;
_optopt = 1;
- ch = carg.charAt(_optopt);
- }
- else {
- ch = carg.charAt(_optopt);
}
+ char ch = carg.charAt(_optopt);
+
// adjust pointer to next character
_optopt += 1;
