Hi Igor,
LGTM++
It is a nice clean. Surprisingly, this code had a lot of typos!
Thanks,
Serguei
On 8/30/20 21:18, David Holmes wrote:
Hi Igor,
Update looks good.
Thanks,
David
On 29/08/2020 4:18 am, Igor Ignatyev wrote:
Hi David,
good point, parseArguments (or rather checkOption) does indeed
validate that passed option is valid and has a valid value, yet for
many options all values are treated as valid, so ill-formed command
lines like `-debugee.vmkeys="${test.vm.opts} ${test.java.opts}
-transport.address=dynamic` won't be spotted by ArgumentParser and
its sub-classes, and I'm afraid in some cases might change tests'
behavior unnoticeably. thus I've decided to add the check that we
always have even number of double quotes:
diff -r 83f273f313aa
test/hotspot/jtreg/vmTestbase/nsk/share/ArgumentParser.java
--- a/test/hotspot/jtreg/vmTestbase/nsk/share/ArgumentParser.java
Thu Aug 27 19:37:51 2020 -0700
+++ b/test/hotspot/jtreg/vmTestbase/nsk/share/ArgumentParser.java
Fri Aug 28 11:16:24 2020 -0700
@@ -156,6 +156,10 @@
arg.append(" ").append(args[++i]);
doubleQuotes += numberOfDoubleQuotes(args[i]);
}
+ if (doubleQuotes % 2 != 0) {
+ throw new TestBug("command-line has odd number of
double quotes:" + String.join(" ", args));
+ }
+
list.add(arg.toString());
}
setRawArguments(list.toArray(String[]::new));
Thanks,
-- Igor
On Aug 27, 2020, at 9:09 PM, David Holmes <[email protected]>
wrote:
Hi Igor,
In case there may be a parsing error and the command-line is
ill-formed, should you abort if you reach the end of the arg list
without finding an even number of double-quotes? Or will
parseArguments already handle that?
Otherwise the changes seem good.
Thanks,
David
-----
On 28/08/2020 12:39 pm, Igor Ignatyev wrote:
http://cr.openjdk.java.net/~iignatyev//8252477/webrev.00/
99 lines changed: 19 ins; 20 del; 60 mod;
Hi all,
could you please review the patch which unblocks the rest of
8219140's (get rid of vmTestbase/PropertyResolvingWrapper) sub-tasks?
background from JBS:
jtreg splits command line by space to get the list of arguments
and there is no way to prevent that (nor thru escaping, nor by
adding quotes). currently, PropertyResolvingWrapper handles that
and joins multiple arguments within double quotes into one
argument before passing it to the actual test class. the only
place where it's needed is in the tests which use
nsk/share/ArgumentParser (or more precisely
nsk.share.jpda.DebugeeArgumentHandler and
nsk/share/jdb/JdbArgumentHandler).
in preparation for PropertyResolvingWrapper removal,
ArgumentParser should be updated to handle the "split" argument on
its own.
I've also taken the liberty to slightly clean up ArgumentParser.
JBS: https://bugs.openjdk.java.net/browse/JDK-8252477
webrev: http://cr.openjdk.java.net/~iignatyev//8252477/webrev.00/
testing: all the tests which use ArgumentParser
(:vmTestbase_nsk_aod :vmTestbase_nsk_jdb :vmTestbase_nsk_jdi
:vmTestbase_nsk_jdw ,:vmTestbase_nsk_jvmti :vmTestbase_vm_compiler
:vmTestbase_vm_mlvm) on {windows,linux,macos}-x64
Thanks,
-- Igor
