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

[Zoltán Majó]

Hi Albert,


thank you for the feedback!

As this is a small change, I think we can push it with only one 
Reviewer's review.

Best regards,


Zoltan

On 10/10/2014 09:21 AM, Albert Noll wrote:
> 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
>>>>>>>
>>>>>>
>>>>
>>
>