RFR (L) 8211899: Remove the NSK_CPP_STUB macros from vmTestbase for jvmti/scenarios/[E-M]
JC Beyler
jcbeyler at google.com
Sat Oct 13 00:37:00 UTC 2018
Hi all,
Any chance for a second reviewer on a Friday? :)
Jc
On Thu, Oct 11, 2018 at 11:03 AM Alex Menkov <alexey.menkov at oracle.com>
wrote:
> got it.
>
> LGTM.
>
> --alex
>
> On 10/10/2018 19:03, JC Beyler wrote:
> > Hi Alex,
> >
> > Thanks for the review! Yes I had seen that too before sending this
> > review out and forked that fix into this:
> > https://bugs.openjdk.java.net/browse/JDK-8211905
> >
> > Which now is merged and I've updated my webrev to reflect this of course.
> >
> > My rationale for not doing it here directly is always the same: keeping
> > the changes to the "spirit" of what the change is trying to do. Those
> > extra casts were a bit out-of-scope and so I just forked the fix in
> 8211905.
> >
> > Thanks!
> > Jc
> >
> > On Wed, Oct 10, 2018 at 5:40 PM Alex Menkov <alexey.menkov at oracle.com
> > <mailto:alexey.menkov at oracle.com>> wrote:
> >
> > Hi Jc,
> >
> > Overall looks good.
> >
> > one minor note:
> >
> >
> test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/events/EM06/em06t001/em06t001.cpp:
> > - jclassName = (jstring) (jstring) (jstring) (jstring) (jstring)
> > (jstring) (jstring) (jstring) (jstring)
> NSK_CPP_STUB3(CallObjectMethod,
> > jni_env, klass,
> > - methodID);
> > + jclassName = (jstring) (jstring) (jstring) (jstring) (jstring)
> > (jstring) (jstring) (jstring) (jstring)
> > jni_env->CallObjectMethod(klass,
> > methodID);
> >
> > Please drop multi "(jstring)"
> >
> > --alex
> >
> > On 10/08/2018 21:21, JC Beyler wrote:
> > > Hi all,
> > >
> > > I am continuing the NSK_CPP_STUB removal with this next webrev.
> > > Webrev: http://cr.openjdk.java.net/~jcbeyler/8211899/webrev.00/
> > <http://cr.openjdk.java.net/%7Ejcbeyler/8211899/webrev.00/>
> > > <http://cr.openjdk.java.net/%7Ejcbeyler/8211899/webrev.00/>
> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8211899
> > >
> > > The change is still straight-forward though, since it is just
> > doing the
> > > same NSK_CPP_STUB removal. However when I was looking at the
> > changes, a
> > > lot of these changes are touching lines with spaces before/after
> > > parenthesis. I've almost never touched the spaces except if I was
> > > refactoring by hand the line at the same time. The rationale
> > being that
> > > the lines will get fixed a few more times and are, at worse,
> > covered by
> > > the bug: https://bugs.openjdk.java.net/browse/JDK-8211335, which
> > I've
> > > commited to doing.
> > >
> > > Two exceptions are here where I pushed out the code into
> > assignments due
> > > to really long lines and complex if structures:
> > > - jvmti/scenarios/hotswap/HS204/hs204t003/hs204t003.cpp
> > >
> > <
> http://cr.openjdk.java.net/%7Ejcbeyler/8211899/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS204/hs204t003/hs204t003.cpp.udiff.html
> >
> > > - jvmti/scenarios/jni_interception/JI01/ji01t001/ji01t001.cpp
> > >
> > <
> http://cr.openjdk.java.net/%7Ejcbeyler/8211899/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/jni_interception/JI01/ji01t001/ji01t001.cpp.udiff.html
> >
> > >
> > > And one exception here where a commented line was doing the
> > out-of-if
> > > assignment so I just uncommented it and used the variable:
> > > - jvmti/scenarios/hotswap/HS301/hs301t001/hs301t001.cpp
> > >
> > <
> http://cr.openjdk.java.net/%7Ejcbeyler/8211899/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS301/hs301t001/hs301t001.cpp.udiff.html
> >
> > >
> > > I've tested the modified changes on my machine, all modified
> > tests pass.
> > >
> > > Let me know what you think,
> > > Jc
> > >
> > > Ps: 2 more of these and we can say good bye to NSK_CPP_STUB*
> > >
> >
> >
> >
> > --
> >
> > Thanks,
> > Jc
>
--
Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181012/2e882971/attachment.html>
More information about the serviceability-dev
mailing list