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<[email protected]> >> 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 ... >>> >>>
