Hi Jc, Thank you for your comment! I updated a webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.01/ > - In runTests; if DebugdUtils implemented Closeable, you could just do a > try-with-resources instead of the finally clause... I created DebugdUtils for convenience class for attach - detach mechanism of debug server. IMHO it is prefer "detach" to "close" in this case. Thanks, Yasumasa 2019年6月5日(水) 11:34 Jean Christophe Beyler <jcbey...@google.com>: > > Hi Yasumasa, > > I'm not an official reviewer but I don't see an issue with the CSR (except > that this seems to be bringing a fork in the tools with some handling remote > and others not). > > However, this code is really repetitive and this is not the place to do a big > refactor probably but we could do a few nits perhaps: > - Instead of every tool calling commonHelp with an additional flag you > could divide into commonHelp and commonHelpWithRemote for the tools and they > both call the current commonHelp with that boolean; so that when we are > looking at the tool code we know what we are getting... So something like: > > private static boolean commonHelp(String mode, boolean canConnectToRemote) { > .. > } > > private static boolean commonHelp(String mode) { > return commonHelp(mode, false); > } > > private static boolean commonHelpWithRemote(String mode) { > return commonHelp(mode, false); > } > > and that way the tools that change are just going from: > - return commonHelp("jmap"); > + return commonHelpWithRemote("jmap"); > > - In the same vein, instead of passing null to the buildAttachArgs; you could > make a variable null: > > - buildAttachArgs(newArgs, pid, exe, core, true); > + String noRemote = null; > + buildAttachArgs(newArgs, pid, exe, core, noRemote, true); > > > - > http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.00/test/hotspot/jtreg/serviceability/sa/sadebugd/DebugdUtils.java.html > Nit: you have empty lines at l64 and l73 > > - > http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.00/test/hotspot/jtreg/serviceability/sa/sadebugd/DebugdConnectTest.java.html > Nit : you have an empty line at l110 > > - In runTests; if DebugdUtils implemented Closeable, you could just do a > try-with-resources instead of the finally clause... > > All of these are details, I just thought I'd mention them :) > Jc > > On Tue, Jun 4, 2019 at 6:44 PM Yasumasa Suenaga <yasue...@gmail.com> wrote: >> >> PING: Could you review them? >> >> > JBS: https://bugs.openjdk.java.net/browse/JDK-8209790 >> > CSR: https://bugs.openjdk.java.net/browse/JDK-8224979 >> > webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.00/ >> >> CSR status is provisional. So I need reviewers both CSR and webrev. >> >> >> Thanks, >> >> Yasumasa >> >> >> 2019年5月29日(水) 22:37 Yasumasa Suenaga <yasue...@gmail.com>: >> > >> > Hi all, >> > >> > Please review this change: >> > >> > JBS: https://bugs.openjdk.java.net/browse/JDK-8209790 >> > CSR: https://bugs.openjdk.java.net/browse/JDK-8224979 >> > webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.00/ >> > >> > In JDK 8 or earlier, some tools (jstack, jmap, jinfo) can connect to >> > debug server (jsadebugd). However it has not done so since JDK 9 because >> > jhsdb cannot accept the attach request to debug server. >> > So I want to introduce new option `--remote` to connect to debug server. >> > >> > I created CSR for this issue. So please review it together. >> > >> > >> > Thanks, >> > >> > Yasumasa > > > > -- > > Thanks, > Jc
