Hi Alex, Done! Anyone else motivated in looking at part 1 out of 4 to remove all JNI_ENV* from our vmTestbase tests?
Webrev: http://cr.openjdk.java.net/~jcbeyler/8210385/webrev.01/ Bug: https://bugs.openjdk.java.net/browse/JDK-8210385 Thanks! Jc On Fri, Sep 7, 2018 at 2:07 PM Alex Menkov <alexey.men...@oracle.com> wrote: > Hi Jc, > > The fix looks good. > The only note: > > > test/hotspot/jtreg/vmTestbase/nsk/jvmti/IsFieldSynthetic/isfldsin003/isfldsin003.cpp > > contains double jvmti initialization: > - res = JNI_ENV_PTR(jvm)->GetEnv(JNI_ENV_ARG(jvm, (void **) &jvmti), > - JVMTI_VERSION_1_1); > + res = jvm->GetEnv((void **) &jvmti, JVMTI_VERSION_1_1); > if (res != JNI_OK || jvmti == NULL) { > printf("Wrong result of a valid call to GetEnv!\n"); > return JNI_ERR; > } > > - res = JNI_ENV_PTR(jvm)->GetEnv(JNI_ENV_ARG(jvm, (void **) &jvmti), > - JVMTI_VERSION_1_1); > + res = jvm->GetEnv((void **) &jvmti, JVMTI_VERSION_1_1); > if (res != JNI_OK || jvmti == NULL) { > printf("Wrong result of a valid call to GetEnv!\n"); > return JNI_ERR; > } > Could you please remove one of them. > > --alex > > On 09/07/2018 12:22, JC Beyler wrote: > > Hi Alex, > > > > Sounds good to me, I'll aim between 40~50 files then :) > > > > Here is the jvmti/A-N* remaining refactoring of the JNI_ENV* / > > JVMTI_ENV* macros: > > > > I have updated the bug to reflect this: > > > > Webrev: http://cr.openjdk.java.net/~jcbeyler/8210385/webrev.01/ > > <http://cr.openjdk.java.net/%7Ejcbeyler/8210385/webrev.01/> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8210385 > > > > Thanks again! > > Jc > > > > > > On Fri, Sep 7, 2018 at 11:51 AM Alex Menkov <alexey.men...@oracle.com > > <mailto:alexey.men...@oracle.com>> wrote: > > > > I think ~40-50 files per fix is my maximum :) > > > > --alex > > > > On 09/07/2018 11:37, JC Beyler wrote: > > > Sounds good to me, would you prefer it in 4 webrevs of 1k > > changes? Or 2 > > > webrevs of 2k changes? > > > > > > I can do either easily, so just let me know; your job as the > > reviewer is > > > harder for these :) > > > Jc > > > > > > On Fri, Sep 7, 2018 at 11:34 AM Alex Menkov > > <alexey.men...@oracle.com <mailto:alexey.men...@oracle.com> > > > <mailto:alexey.men...@oracle.com > > <mailto:alexey.men...@oracle.com>>> wrote: > > > > > > Hi Jc, > > > > > > I'd prefer to see the fix splitted to smaller parts. > > > The changes are quite simple, but it's just too many files, > > so it's > > > impossible (at least for me) to review them accurately. > > > > > > --alex > > > > > > On 09/07/2018 10:53, JC Beyler wrote: > > > > Hi all, > > > > > > > > I'm taking the risk to perhaps do something too huge but > > as it is > > > the > > > > same as the previous ones (and the last one!): > > > > > > > > It's 4k of changes, for that I apologize. I can divide it > > up in 2/3 > > > > parts again if you want or we can just do it in one go > > since now > > > some > > > > reviewers know what to expect. There still is a bit of > > > refactoring to > > > > the vmTestbase but this will in essence finish: > > > > - Removing the JNI_ENV* and JVMTI_ENV* macros for the > > vmTestbase > > > > > > > > Webrev: > > http://cr.openjdk.java.net/~jcbeyler/8210385/webrev.00/ > > <http://cr.openjdk.java.net/%7Ejcbeyler/8210385/webrev.00/> > > > <http://cr.openjdk.java.net/%7Ejcbeyler/8210385/webrev.00/> > > > > <http://cr.openjdk.java.net/%7Ejcbeyler/8210385/webrev.00/ > > > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8210385 > > > > > > > > Thanks for your reviews and let me know if you would > > prefer me to > > > cut it > > > > up like I did the Get[A-F] & Get[G-Z] ones, > > > > Jc > > > > > > > > Ps: for the curious, then we can start cleaning up the > native > > > code a bit > > > > more sanely (example: JDK-8191519) and there will be one > > more webrev > > > > removing #ifdef cplusplus lingering in the vmTestbase > > (JDK-8210481). > > > > > > > > > > > > > > > > -- > > > > > > Thanks, > > > Jc > > > > > > > > -- > > > > Thanks, > > Jc > -- Thanks, Jc
