[9] RFR(S): 8059847: complement JDK-8055286 and JDK-8056964 changes

[Albert Noll]

Hi Zoltan,


On 10/09/2014 10:20 AM, Zoltán Majó wrote:
> Hi Vladimir,
>
>
> thank you for your feedback!
>
> On 10/08/2014 05:27 PM, Vladimir Kozlov wrote:
>> You don't need /othervm if there  are no additional flags.
>
> OK, I removed it.
>
>> Instead of /startup/ I would suggest /oracle/ subdir name since we 
>> testing code in compilerOracle.
>
> OK, I moved the test to the 'oracle' subdirectory.
>
>> 42     // Type (1) is used to check if a flag "someflag" is enabled 
>> for a
>> 43     // method.
>>
>> I would say:
>>
>> 42     // Type (1) is used to enable a boolean flag for a method.
>
> I updated the comment in the test (and also in compilerOracle.cpp on 
> line 732).
>
>> Should it be "bool,MyBoolOption,100"?:
>>
>>  135             // wrong value for bool flag
>>  136 "-XX:CompileCommand=option,Test::test,bool,MyBoolOption,intx,100",
>
> I changed the test to what you've suggested.
>
>> Same problem with next test which does not match the comment:
>>
>> 140             // intx flag name missing
>> 141 
>> "-XX:CompileCommand=option,Test::test,bool,MyBoolOption,false,intx,100",
>
> I changed this test as well.
>
> I've also added three new tests to TYPE_2_INVALID_ARGUMENTS. The new 
> tests are very similar to the ones in webrev.01.
>
>> Please, separate next checks to be clear which one is incorrect:
>>
>>  179         if (TYPE_1_ARGUMENTS.length != 
>> TYPE_1_EXPECTED_OUTPUTS.length
>>  180             || TYPE_2_ARGUMENTS.length != 
>> TYPE_2_EXPECTED_OUTPUTS.length) {
>>  181             throw new RuntimeException("Test is set up 
>> incorrectly: length of arguments and expected outputs does not match.");
>>  182         }
>
> I separated the checks and updated the error messages.
>
> Here is the new webrev:
>
> http://cr.openjdk.java.net/~zmajo/8059847/webrev.02/
>
That looks good to me (not a reviewer).

Best,
Albert

> Thank you!
>
> Best regards,
>
>
> Zoltan
>
>>
>> Thanks,
>> Vladimir
>>
>> On 10/8/14 7:22 AM, Zoltán Majó wrote:
>>> Hi David and Vladimir,
>>>
>>>
>>> thank you for your feedback.
>>>
>>> On 10/07/2014 10:46 PM, Vladimir Kozlov wrote:
>>>> On 10/7/14 11:06 AM, David Chase wrote:
>>>>
>>>>> Do we need to add a test (? what would it look like?) to ensure 
>>>>> that it works?
>>>>
>>>> Yes, please, add a regression test.
>>>
>>> I added a regression test 
>>> ('compiler/startup/CheckCompileCommandOption.java'). Here is the 
>>> updated webrev:
>>>
>>> http://cr.openjdk.java.net/~zmajo/8059847/webrev.01/
>>>
>>> Thank you and best regards,
>>>
>>>
>>> Zoltan
>>>
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>>>
>>>>> David
>>>>>
>>>>> On 2014-10-07, at 1:53 PM, Zoltán Majó <zoltan.majo at oracle.com> 
>>>>> wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>>
>>>>>> please review the following small patch.
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8059847
>>>>>>
>>>>>> Problem: CompileCommand=option does not support flags of type 
>>>>>> double.
>>>>>>
>>>>>> Solution: Add support for flags of type double. Output:
>>>>>>
>>>>>> java 
>>>>>> -XX:CompileCommand=option,Test::test,ccstrlist,MyListOption,_foo,_bar 
>>>>>>
>>>>>> -XX:CompileCommand=option,Test::test,ccstr,MyStrOption,_foo
>>>>>> -XX:CompileCommand=option,Test::test,bool,MyBoolOption,false
>>>>>> -XX:CompileCommand=option,Test::test,intx,MyIntxOption,-1 
>>>>>> -XX:CompileCommand=option,Test::test,uintx,MyUintxOption,1
>>>>>> -XX:CompileCommand=option,Test::test,MyFlag 
>>>>>> -XX:CompileCommand=option,Test::test,double,MyDoubleOption,1.123 
>>>>>> -version
>>>>>>
>>>>>> CompilerOracle: option Test.test const char* MyListOption = '_foo 
>>>>>> _bar'
>>>>>> CompilerOracle: option Test.test const char* MyStrOption = '_foo'
>>>>>> CompilerOracle: option Test.test bool MyBoolOption = false
>>>>>> CompilerOracle: option Test.test intx MyIntxOption = -1
>>>>>> CompilerOracle: option Test.test uintx MyUintxOption = 1
>>>>>> CompilerOracle: option Test.test bool MyFlag = true
>>>>>> CompilerOracle: option Test.test double MyDoubleOption = '1.123000'
>>>>>> java version "1.9.0-ea"
>>>>>> Java(TM) SE Runtime Environment (build 1.9.0-ea-b33)
>>>>>> Java HotSpot(TM) 64-Bit Server VM (build 
>>>>>> 1.9.0-fastdebug-internal, mixed mode)
>>>>>>
>>>>>>
>>>>>> Webrev: http://cr.openjdk.java.net/~zmajo/8059847/webrev.00/
>>>>>>
>>>>>> Testing: JPRT
>>>>>>
>>>>>> Thank you and best regards,
>>>>>>
>>>>>>
>>>>>> Zoltan
>>>>>>
>>>>>
>>>
>