code review for JLI/JPLIS test (7191322)

[Daniel D. Daugherty]

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 ...
>>
>>