code review for JLI/JPLIS test (7191322)

[Daniel D. Daugherty]

Dmitry,

Thanks for the quick review!

Replies embedded below.


On 8/15/12 11:39 AM, Dmitry Samersoff wrote:
> Dan,
>
> VerifyLocalVariableTableOnRetransformTest.sh

Just FYI that VerifyLocalVariableTableOnRetransformTest.sh
is a copy of one of the existing JLI .sh files with minor
tweaking specific for this test. All of the comments below
are on the "boiler plate" stuff that I copied.


> 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've never heard of a shell not handling the above
correctly when double quotes are used. This is an
idiom that is in all of the JLI .sh tests and probably
in all the JDI .sh tests. I have seen the following:

     if [ x${TESTJAVA} = x ]

in some other scripts and that drives me bonkers.

I'm planning to keep this one as is, if that's OK
with you.


> 2.
>
> ll. 69
>
> it's better to add quotes around 0 or
> use -eq instead of =
>
>
> besides that looks good for me.

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.


Again, thanks for the quick review! Please let me know
if you are OK with the above resolutions.

Dan


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