code review for JLI/JPLIS test (7191322)

[Kelly O'Hair]

i m fine wit it

-kto

On Aug 16, 2012, at 3:13 PM, Daniel D. Daugherty wrote:

> Kelly,
> 
> Thanks for the review. Replies embedded below.
> 
> 
> On 8/15/12 11:28 PM, Kelly Ohair wrote:
>> 
>> Sent from my iPhone
>> 
>> On Aug 15, 2012, at 19:39, Dmitry Samersoff<Dmitry.Samersoff at oracle.com>  wrote:
>> 
>>> Dan,
>>> 
>>> VerifyLocalVariableTableOnRetransformTest.sh
>>> 
>>> 1.
>>> 
>>> if [ "${TESTJAVA}" = "" ]
>>> 
>>> Some shells doesn't handle it correctly.
>>> It's more safe to do
>>> 
>>> if [ "x${TESTJAVA}" = "x" ]
>>> 
>>> (the same is for other conditions below)
>>> 
>> i myself find the x trick ugly
>> 
>> i have not had a need to do the x trick for 10 years
>> 
>> what sh or bash shell that we use has this problem still??????
> 
> Dmitry and I hashed this one out. He is OK with what I have.
> 
> 
>>> 2.
>>> 
>>> ll. 69
>>> 
>>> it's better to add quotes around 0 or
>>> use -eq instead of =
>> yup i agree there
> 
> This is what I wrote to Dmitry:
> 
>> This is also a pretty common idiom:
>> 
>>    if [ "$result" = 0 ]; then
>> 
>> and I find a bunch of uses of it in existing JLI tests.
>> I can't find any use of '-eq' at all in the JLI tests.
>> In the JDI tests, I find both '-eq' and the above both
>> with and without quotes, but mostly the later.
>> 
>> I'm planning to keep this one as is, if that's OK
>> with you. 
> 
> 
> He is OK with what I have.
> 
> Again, thanks for the review! Please let me know
> if you are OK with the above resolutions.
> 
> Dan
> 
> 
>>> besides that looks good for me.
>>> 
>>> -Dmitry
>>> 
>>> 
>>> On 2012-08-15 20:58, Daniel D. Daugherty wrote:
>>>> Greetings,
>>>> 
>>>> I wrote a test for the following bug:
>>>> 
>>>>    7064927 4/4 retransformClasses() does not pass in
>>>>                LocalVariableTable of a method
>>>> 
>>>> a long time ago. 7064927 was fixed in the hotspot repo back in
>>>> HSX-23-B09 by Thomas W. and Coleen, but the test was never pushed
>>>> to the JDK repo. The java.lang.instrument (JLI) tests live in the
>>>> JDK repo.
>>>> 
>>>> I'm using the following bug:
>>>> 
>>>>    7191322 4/4 add test for 7064927 to java.lang.instrument
>>>> 
>>>> to get the test into the JDK8 T&L repo. Here is the webrev URL:
>>>> 
>>>>    http://cr.openjdk.java.net/~dcubed/7191322-webrev/0/
>>>> 
>>>> Thanks, in advance, for any comments.
>>>> 
>>>> Dan
>>>> 
>>>> P.S.
>>>> The new test has been executed and passes on all platforms
>>>> supported by JPRT except for MacOS X. There is a temporary
>>>> build issue on MacOS X at the moment.
>>> 
>>> -- 
>>> Dmitry Samersoff
>>> Java Hotspot development team, SPB04
>>> * There will come soft rains ...
>>> 
>>>