Looks good for me! -Dmitry
On 2012-04-05 15:43, Markus Grönlund wrote: > Dan, Dmitry, all, > > Thanks for the feedback so far, I have updated the change, please see this > update: > > Webrev: > http://cr.openjdk.java.net/~mgronlun/7154809/webrev06/ > > Bug: > http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7154809 > > > > The use case we are trying to support is the parsing of VM Options being > passed in via the "options=" > > These VM options will either be: > > 1. undecorated (-XX:+PrintVMOptions) > 2. double quoted ("-XX:+PrintVMOptions") > 3. single quoted ('-XX:+PrintVMOptions') > 4. nested quoted ('"-XX:+PrintVMOptions"', or "'-XX:+PrintVMOptions'") > > Above change results in: > > -XX:+PrintVMOptions > > For all the above. > > > Example (the extra \ qualifiers is to avoid shell resolution for windows > commandline): > > java TestApplication \"-client\" \"-Xmixed\" \"-XX:+PrintVMOptions\" > \"'-XX:ExtendedExampleOptions=value1=true,value2=true,value3=false'\" > Printing raw arguments: > "-client" > "-Xmixed" > "-XX:+PrintVMOptions" > "'-XX:ExtendedExampleOptions=value1=true,value2=true,value3=false'" > > Printing raw arguments stringed together: "-client" "-Xmixed" > "-XX:+PrintVMOptions" > "'-XX:ExtendedExampleOptions=value1=true,value2=true,value3=false'" > > strings after splitStringAtNonEnclosedWhitespace(): > > "-client" > now sending string: "-client" off to isEnclosed() > After processing string in isEnclosed: > -client > > "-Xmixed" > now sending string: "-Xmixed" off to isEnclosed() > After processing string in isEnclosed(): > -Xmixed > > "-XX:+PrintVMOptions" > now sending string: "-XX:+PrintVMOptions" off to isEnclosed() > After processing string in isEnclosed(): > -XX:+PrintVMOptions > > "'-XX:ExtendedExampleOptions=value1=true,value2=true,value3=false'" > now sending string: > "'-XX:ExtendedExampleOptions=value1=true,value2=true,value3=false'" off to > isEnclosed() > After processing string in isEnclosed(): > -XX:ExtendedExampleOptions=value1=true,value2=true,value3=false > > Final result > -client -Xmixed -XX:+PrintVMOptions > -XX:ExtendedExampleOptions=value1=true,value2=true,value3=false > > > Thank you > Markus > > > p.s. > > Dan, thanks for the esoteric example. The output from the example string > (using debug version of code): > > The following is a single quote: ' Do you see it? > > Is (depending on how you pass it in (quoted or unquoted)): > > D:\devjava\1.6.0\projects\enclosed\source>java TestApplication a single > quote: ' do you see it? > > Printing raw arguments: > > a > single > quote: > ' > do > you > see > it? > > Printing raw arguments stringed together: a single quote: ' do you see it? > > strings after splitStringAtNonEnclosedWhitespace: > > a > now sending string: a off to isEnclosed() > After processing string in isEnclosed(): > a > > single > now sending string: single off to isEnclosed() > After processing string in isEnclosed(): > single > > quote: > now sending string: quote: off to isEnclosed() > After processing string in isEnclosed(): > quote: > > ' > now sending string: ' off to isEnclosed() > After processing string in isEnclosed(): > ' > > do > now sending string: do off to isEnclosed() > After processing string in isEnclosed: > do > > you > now sending string: you off to isEnclosed() > After processing string in isEnclosed(): > you > > see > now sending string: see off to isEnclosed() > After processing string in isEnclosed(): > see > > it? > now sending string: it? off to isEnclosed() > After processing string in isEnclosed(): > it? > > Final result > a single quote: ' do you see it? > > D:\devjava\1.6.0\projects\enclosed\source>java TestApplication \"a single > quote: ' do you see it?\" > > Printing raw arguments: > "a > single > quote: > ' > do > you > see > it?" > > Printing raw arguments stringed together: "a single quote: ' do you see it?" > > strings after splitStringAtNonEnclosedWhitespace > "a single quote: ' do you see it?" > now sending string: "a single quote: ' do you see it?" off to isEnclosed() > After processing string in isEnclosed(): > a single quote: ' do you see it? > > Final result > a single quote: ' do you see it? > > > D:\devjava\1.6.0\projects\enclosed\source>java TestApplication 'a single > quote: \" do you see it?' > > Printing raw arguments > 'a > single > quote: > " > do > you > see > it?' > Printing raw arguments stringed together: 'a single quote: " do you see it?' > strings after splitStringAtNonEnclosedWhitespace > 'a single quote: " do you see it?' > now sending string: 'a single quote: " do you see it?' off to isEnclosed() > After processing string in isEnclosed: > a single quote: " do you see it? > > Final result > a single quote: " do you see it? > > Cheers > Markus > > > > > > > > > >> -----Original Message----- >> From: Daniel D. Daugherty >> Sent: den 29 mars 2012 22:30 >> To: [email protected] >> Cc: [email protected] >> Subject: Re: RFR(XS): 7154809 JDI: update JDI/JDB debugee commandline >> option parsing (allow nested comma delimited options) + sponsor request >> >> On 3/28/12 3:13 PM, Markus Grönlund wrote: >>> Dan, Dmitry, everybody, >>> >>> Thank you so far for your input. >>> >>> Dan, you are absolutely right in your reflection about "<word1> >> <word2>" combined with 'split("\\s+")' regexp - this will split the >> strings to"<word1> and<word2>" which does not work. We should also try >> to cover this. >>> >>> This turned out a lot trickier than I originally thought - I have >> tried to come up with some good regexps that would combine the >> combination of what's needed (nested spaces, nested quotations >> (different quotations), several levels...), however I did not get it to >> work correctly in all combinations with the regexps - I eventually >> decided to take control over this down to the char level - hence this >> suggestion with a C-like array iteration (maybe slow yeah, however I >> found this so much more debuggable and actually a chance to see what's >> going on. Also, this parsing for the "values"-string associated with >> the "option=" is only parsed once at startup, just when getting the >> correct VM Option parameters for/of the debugee). >>> >>> Updated webrev: >>> http://cr.openjdk.java.net/~mgronlun/7154809/webrev05/ >> >> src/share/classes/com/sun/tools/example/debug/tty/VMConnection.java >> nit lines 118-119: extra space after '*' >> >> line 178: what happens when null is passed to format()? >> lines 182, many others - missing space after '//' >> >> line 217 - please consider changing comment to: >> >> // this DOUBLEQ does not complete this enclosing - skip to >> next >> >> line 224: please change comment to match line 256: >> >> //set up the target char for ending enclosing >> >> line 229 - please consider changing comment to: >> >> // this DOUBLEQ does not start a new enclosing - skip to next >> >> line 233: add "is there" to beginning of comment to match line 201 >> >> line 249 - please consider changing comment to: >> >> // this SINGLEQ does not complete this enclosing - skip to >> next >> >> line 261 - please consider changing comment to: >> >> // this SINGLEQ does not start a new enclosing - skip to next >> >> line 286: This check might be hit for an input string that looks >> like: >> >> The following is a single quote: ' Do you see it? >> >> Pretty bogus example so throwing that exception might be the >> right thing to do. The idea I have is that if you have a >> singleton DOUBLEQ or SINGLEQ in the input, then you don't >> have an enclosing state... May not be worth the hassle... >> >> Dan >> >> >> >>> There was eventually some more code ending up in this one (trying to >> be very explicit here) - it might make more sense to refactor this to >> outside of VMConnection.java someplace - if you have any ideas... >>> >>> Some of the word combinations supported with this change: >>> >>> "'<word1> <word2>'"<word3> '"<word4> <word5>"'"<word6> <word7>" >> '<word8> <word9>'<word10> "<wor"d11>" '<word'12>''<wor"d11>' >> "<wor"d'12>" "<word13> " .... >>> >>> I think I should need to track the changes to the options >> passing/parsing test code in nsk testbase as well in a separate bug for >> correlation with these changes - might make more sense. I will file a >> test bug on the updates needed on the test side of things. >>> >>> Thanks again for your help >>> Markus >>> >>> >>> >>>> -----Original Message----- >>>> From: Daniel D. Daugherty >>>> Sent: den 27 mars 2012 21:29 >>>> To: Markus Grönlund >>>> Cc: Dmitry Samersoff; [email protected]; hotspot- >>>> [email protected] >>>> Subject: Re: RFR(XS): 7154809 JDI: update JDI/JDB debugee >> commandline >>>> option parsing (allow nested comma delimited options) + sponsor >> request >>>> >>>> On 3/27/12 11:56 AM, Markus Grönlund wrote: >>>>> Hi again all, >>>>> >>>>> Thanks for the input Dan and Dmitry. >>>>> >>>>> I took a closer look at this, we can actually pass options to the >> VM >>>> which could involve filenames and these *could* have quotation >>>> qualifiers embedded (ccstr type parameters); this means the original >>>> suggestion will not work correctly, in doing >> value.replaceAll("['\"]", >>>> ""), as it will also remove all quotations for any embedded >> VMOptions >>>> of ccstr type... >>>>> Neither will Dmitrys suggestion of removing just the edge >> quotations >>>> work, at least not as-is, it would need foreach value iteration. >>>>> I have updated the webrev with such a modification (which will >>>> recursively remove quotations from outside-in for each value). This >>>> will allow for "'test'", '" test "', "" test "", ''test'', "test", >>>> 'test' ...etc. >>>>> Please see updated webrev here: >>>>> >>>>> http://cr.openjdk.java.net/~mgronlun/7154809/webrev04/ >>>> src/share/classes/com/sun/tools/example/debug/tty/VMConnection.java >>>> I like the newer version and the concept of "enclosing >> quotes". >>>> One thing I'm not quite sure about is the use of 'split()'. >> What >>>> will the new logic do with the following value? >>>> >>>> "<word1> <word2>" >>>> >>>> I'm pretty sure the split() call will break the above into two >>>> tokens: '"<word1>' and'<word2>"'. The subsequent calls to >>>> isEnclosed() will return false and we won't strip the double >>>> quotes. Maybe I'm misunderstanding how 'split("\\s+")' >> works... >>>> >>>> BTW, this options parsing stuff is exceedingly difficult to get >>>> working right and working the same on all platforms. >>>> >>>> Dan >>>> >>>> >>>> >>>>> Thanks you so much for your help >>>>> >>>>> Cheers >>>>> Markus -- Dmitry Samersoff Java SE development team, SPB04 * There will come soft rains ...
