Still looking for 2nd reviewer
--alex
On 08/24/2020 13:50, Alex Menkov wrote:
${subj}
Need 2nd reviewer
--alex
On 08/19/2020 16:46, serguei.spit...@oracle.com wrote:
Thank you for the update, Alex!
It looks good.
Thanks,
Serguei
On 8/19/20 16:35, Alex Menkov wrote:
Updated webrev:
http://cr.openjdk.java.net/~amenkov/jdk16/jdb_options/webrev.02/
--alex
On 08/19/2020 16:14, serguei.spit...@oracle.com wrote:
On 8/19/20 15:11, Alex Menkov wrote:
Hi Serguei,
thank you for the feedback.
On 08/19/2020 13:58, serguei.spit...@oracle.com wrote:
Hi Alex,
Sorry, I've overlooked this request for review.
The fix looks good in general.
http://cr.openjdk.java.net/~amenkov/jdk16/jdb_options/webrev/src/jdk.jdi/share/classes/com/sun/tools/example/debug/tty/VMConnection.java.frames.html
81 private Map <String, com.sun.jdi.connect.Connector.Argument>
parseConnectorArgs(Connector connector,
82 String argString,
83 String extraOptions) {
To make it more elegant I'd suggest to place the returned type on
a separate line like below:
private Map<String, com.sun.jdi.connect.Connector.Argument>
parseConnectorArgs(Connector connector, String argString, String
extraOptions) {
Do you mean second line indent should be the same as 1st?
or make it 8 spaces:
private Map<String, com.sun.jdi.connect.Connector.Argument>
parseConnectorArgs(Connector connector, String argString,
String extraOptions) {
No indent is needed, I think.
My suggestion is to use extra line for method return type instead of
method arguments.
127 sb.append(extraOptions).append(" ");
128 // set extraOptions to null to not set it again
129 extraOptions = null;
What about rewording the comment like below? :
// set extraOptions to null to avoid appending it again
ok.
165 if (extraOptions != null) {
166 // there was no "option" specified in argString
167 Connector.Argument argument = arguments.get("options");
168 if (argument != null) {
169 argument.setValue(extraOptions);
170 }
171 }
Should the "option" in the comment be replaced with "options"?
right.
What if the argument at line 167 was set to null?
Will the extraOptions be ignored in such a case?
extraOptions makes sense only for CommandLineLaunch connector which
launches new VM (and only this connector has "options" argument).
Other connectors (attach or listen) connect to existing VM and
cannot set its options.
Okay, thank you for explanation.
Thanks,
Serguei
http://cr.openjdk.java.net/~amenkov/jdk16/jdb_options/webrev/test/jdk/com/sun/jdi/JdbOptions.java.html
This line is probably not needed anymore:
157 //jdb.quit();
will delete.
--alex
Thanks,
Serguei
On 8/7/20 15:09, Alex Menkov wrote:
Hi all,
please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8234808
webrev:
http://cr.openjdk.java.net/~amenkov/jdk16/jdb_options/webrev/
Some background:
when jdb launches debuggee process it passes java options from
"options" value for CommandLineLaunch connector and forward
options specified before command.
The fix solves several discovered issues:
- proper handling of java options with spaces
- if both way are used to specify java options, forwarded options
override options from "options" value
VMConnection class implements tricky logic for "options" field
parsing for JFR needs (handling of single and double quotes). I
decided to keep it as is to avoid massive test failures with JFR
(there is no test coverage for this functionality and I'm not
sure I understand all requirements).
--alex
