Re: RFR (XL) 8210385: Clean up JNI_ENV_ARG and factorize the macros for remaining vmTestbase/jvmti tests

[Alex Menkov]

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