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

[David Holmes]

Hi Per,

On 10/04/2017 9:22 PM, Per Liden wrote:
> Hi David,
>
> On 2017-04-10 13:03, David Holmes wrote:
>> On 10/04/2017 5:40 PM, Per Liden wrote:
>>> Hi David,
>>>
>>> On 2017-04-10 09:27, David Holmes wrote:
>>>> Hi Per,
>>>>
>>>> On 10/04/2017 5:05 PM, Per Liden wrote:
>>>>> Hi,
>>>>>
>>>>> 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 :) ).

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" 
test should be testing. :) If not, then test is okay.

Thanks,
David

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