[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR(S): JDK-8059038 Create new launcher for SA tools
From: Jaroslav Bachorik <jaroslav.bachorik () oracle ! com>
Date: 2015-07-23 15:01:30
Message-ID: 55B101CA.3030403 () oracle ! com
[Download RAW message or body]
Dmitry, thanks for undertaking this.
Thumbs up from me!
-JB-
On 17.7.2015 16:29, serguei.spitsyn@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.spitsyn@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.spitsyn@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.spitsyn@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.spitsyn@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.spitsyn@oracle.com wrote:
> > > > > > > > > > > Hi Dmitry,
> > > > > > > > > > >
> > > > > > > > > > > Some quick minor comments.
> > > > > > > > > > >
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > 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 }
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > > \
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > 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
> > > > > > > > > > > >
> > > > > > > > > > > >
> >
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic