Diagnostic command fixes

[Frederic Parain]

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