Serguei, David, thanks for your reviews, pushed.
-- Igor > On Sep 1, 2020, at 1:09 AM, [email protected] wrote: > > 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 >>> >
