|
Hi Jc,
+1
Thanks,
Serguei
On 9/7/18 14:07, Alex Menkov 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
|
