RFR (L) 8211899: Remove the NSK_CPP_STUB macros from vmTestbase for jvmti/scenarios/[E-M]

Alex Menkov alexey.menkov at oracle.com
Thu Oct 11 18:03:51 UTC 2018


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


More information about the serviceability-dev mailing list