RFR(s): 8178364: Command-line flags of type double should accept integer values

[Per Liden]

Hi David,

On 2017-04-10 14:49, David Holmes wrote:
[...]
>>>>>> For convenience, command-line flags of type double should accept
>>>>>> integer
>>>>>> values (as well as the normal double values). This would allow
>>>>>> users to
>>>>>> write the short form, e.g:
>>>>>>
>>>>>> -XX:G1ConcMarkStepDurationMillis=10
>>>>>>
>>>>>> instead of always having to do:
>>>>>>
>>>>>> -XX:G1ConcMarkStepDurationMillis=10.0
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8178364
>>>>>> Webrev: http://cr.openjdk.java.net/~pliden/8178364/webrev.0/
>>>>>> Testing: JTreg test added, passes jprt
>>>>>
>>>>> I don't understand why we aren't using set_fp_numeric_flag in this
>>>>> case
>>>>> - and surely that already handles this case ??
>>>>
>>>> With the way this code is structured we only end up in
>>>> set_fp_numeric_flag() if the string looks like a float to begin with,
>>>> i.e. has the format "<flag>=<num>.<num>". If the flag looks like
>>>> "<flag>=<num>" we end up in set_numeric_flag(). Have a look at the call
>>>> sites for set_fp_numeric_flag() and set_numeric_flag() and you'll see.
>>>
>>> That's somewhat odd - shouldn't the type of the flag dictate how we try
>>> to parse it ??
>>
>> Indeed, Arguments::parse_argument() does things a bit backwards if you
>> ask me, by "guessing" what's on the other side of the "=" instead of
>> letting the flag type dictate how it's parsed. Maybe there is (or was) a
>> reason for doing so that I don't see.
>>
>> Anyway, I think my patch is in line with the style/intent of the current
>> code. Overhauling Arguments::parse_argument() is probably a good idea,
>> but I think that's a separate enhancement.
>
> Okay - I agree that what you have is a logical extension of the logic in
> set_numeric_flag. (but the logic above that is really quite odd to me :) ).

Yep, agreed.

>
> Was wondering if the testcase could somehow be added to one of the
> existing options validation tests? I admit I couldn't see where to try
> and fit it, but it sounds like something that an "options validation"

I had a look at those, but it's not clear to me either how that would 
fit in. It seems those tests are more focused on value range validation, 
rather than value format testing. In test/runtime/CommandLine we 
currently have a number of other tests for value format checking 
(BooleanFlagWithInvalidValue, FlagWithInvalidValue, etc), so I modeled 
my new test after those.

> test should be testing. :) If not, then test is okay.

Ok, thanks for reviewing.

cheers,
Per

>
> Thanks,
> David
>
>> cheers,
>> Per
>>
>>>
>>> David
>>>
>>>> cheers,
>>>> Per
>>>>
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>> cheers,
>>>>>> Per
>>>>>>