code review for JLI/JPLIS test (7191322)

[Dmitry Samersoff]

Dan,

I'm OK with leaving everything as is -

>> if [ "${TESTJAVA}" = "" ]

I did a quick check  and construction above
works on linux, cygwin, Sol10, Sol8

- so I was paranoid here.

-Dmitry

On 2012-08-16 00:38, Daniel D. Daugherty wrote:
> 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.
>>


-- 
Dmitry Samersoff
Java Hotspot development team, SPB04
* There will come soft rains ...