Diagnostic command fixes

[Nils Loodin]

Thank you for the suggestion, I will change the comment!

/Nisse

On Feb 15, 2012, at 10:11 , Frederic Parain wrote:

> Thanks for the clarification, it looks perfectly correct to me.
> But I think the comment needs some changes, may be something like
> that:
> 
> /* NOTE: Some argument types doesn't require a value,
> * for instance boolean arguments: "enableFeatureX" is
> * equivalent to "enableFeatureX=true". In these cases,
> * str will be null. This is perfectly valid.
> * All argument types must perform null checks on str.
> */
> 
> This covers both the case of the JVM command line arguments
> parsing and the diagnostic command arguments parsing.
> 
> Fred
> 
> On 02/15/12 10:00 AM, Nils Loodin wrote:
>> Ah, I think that the comment was perhaps not clear. The way we specify command-line options today is like this
>> 
>> java -XX:SomeOption=optionOne=valueOne,optionTwo=valuetwo
>> 
>> ie, the diagnostic command parser would see / parse everything after the first equals sign.
>> 
>> However, with options that are boolean, you don't need the boolOption=value syntax, you only need something that would look like
>> 
>> java -XX:SomeOption=boolOption,optionTwo=valueTwo
>> 
>> But then, str would be null, which would be legal, and not something we could/should guard against..
>> Hope this was a bit clearer.
>> 
>> /Nils
>> 
>> 
>> On Feb 14, 2012, at 19:31 , Frederic Parain wrote:
>> 
>>>  31   /* NOTE:Some argument types doesn't require a specifier,
>>>  32    * such as boolean (for instance "Options=setSomething". Notice the
>>>  33    * difference between this and "Options=setSomething=true".)
>>>  34    * In these cases, str will be null. This is perfectly valid.
>>>  35    * But other argument types will have to do null checks.
>>>  36    */
>>> 
>>> Looks like a bug rather than a feature.
>>> 
>>> "Options=setSomething=true" is not a syntax originally supported
>>> by the DCmdParser.
>>> 
>>> "Options,setSomething=true" is correct with the comma separator.
>>> or
>>> "Options="setSomething=true" is also correct.
>>> 
>>> But "Options=setSomething=true" should have been rejected.
>>> Unfortunately, this error syntax doesn't seem detected.
>>> 
>>> Fred
>>> 
>>> On 14/02/2012 18:33, Nils Loodin wrote:
>>>> Forgot an important point - the testing
>>>> 
>>>> First I ran all the testing we have for the tracing framework that makes use of these commands.
>>>> I have also debugged through with frivolous command lines to make sure we don't do anything bad, like crash.
>>>> Also I've manually tried to use all the combinations of the command line arguments I can think of that should work, and that shouldn't.
>>>> 
>>>> Regards
>>>> Nils Loodin
>>>> 
>>>> On Feb 14, 2012, at 18:29 , Nils Loodin wrote:
>>>> 
>>>>> Thanks all for suggestions on improvements.
>>>>> I have an updated webrev here:
>>>>> http://cr.openjdk.java.net/~nloodin/7145243/webrev.01/
>>>>> 
>>>>> Regards
>>>>> Nils Loodin
>>>>> 
>>>>> PS: Note - Frederic Parain has graciously taken upon himself to sponsor this. Many thanks!
>>>>> 
>>>>> DS
>>>>> 
>>>>> On Feb 13, 2012, at 22:58 , Nils Loodin wrote:
>>>>> 
>>>>>> Hey all!
>>>>>> 
>>>>>> The new diagnostic command parser needs some additional specializations for time and bytes, here included.
>>>>>> Also a few fixes for crashes for some combinations of commandlines.
>>>>>> 
>>>>>> Tested by throwing a lot of different arguments on the parser, also by running the tests in sun/tools/jcmd.
>>>>>> 
>>>>>> http://cr.openjdk.java.net/~nloodin/7145243/webrev.00/
>>>>>> 
>>>>>> I would also need a sponsor to get this in..
>>>>>> 
>>>>>> Regards
>>>>>> Nils Loodin
>>>>> 
>>>> 
>>> 
>>> --
>>> Frederic Parain - Oracle
>>> Grenoble Engineering Center - France
>>> Phone: +33 4 76 18 81 17
>>> Email: Frederic.Parain at oracle.com
>> 
> 
> -- 
> Frederic Parain - Oracle
> Grenoble Engineering Center - France
> Phone: +33 4 76 18 81 17
> Email: Frederic.Parain at Oracle.com
>