RFR (XL) 8210385: Clean up JNI_ENV_ARG and factorize the macros for remaining vmTestbase/jvmti tests
JC Beyler
jcbeyler at google.com
Tue Sep 11 14:43:26 UTC 2018
Thanks both for the reviews :)
Jc
On Mon, Sep 10, 2018 at 10:04 PM David Holmes <david.holmes at oracle.com>
wrote:
> Hi JC,
>
> I skimmed through this and it seems fine to me.
>
> Thanks,
> David
>
> On 11/09/2018 9:13 AM, JC Beyler wrote:
> > 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/
> > <http://cr.openjdk.java.net/%7Ejcbeyler/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.menkov at oracle.com
> > <mailto:alexey.menkov at 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/>
> > > <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.menkov at oracle.com <mailto:alexey.menkov at oracle.com>
> > > <mailto:alexey.menkov at oracle.com
> > <mailto:alexey.menkov at 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.menkov at oracle.com <mailto:alexey.menkov at oracle.com>
> > <mailto:alexey.menkov at oracle.com <mailto:alexey.menkov at oracle.com>>
> > > > <mailto:alexey.menkov at oracle.com
> > <mailto:alexey.menkov at oracle.com>
> > > <mailto:alexey.menkov at oracle.com
> > <mailto:alexey.menkov at 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/>
> > > > >
> > <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
>
--
Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180911/bb5b14c9/attachment.html>
More information about the serviceability-dev
mailing list