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.men...@oracle.com
<mailto: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/>
> <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>
> <mailto: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>>
> > <mailto: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/>
> > >
<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
