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

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Wed Oct 17 22:45:58 UTC 2018


Hi Jc,

++LGTM.

Sorry for being late.
This takes a lot of efforts - thank you so much for being so patient!

Thanks,
Serguei


On 10/11/18 11:03, Alex Menkov 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



More information about the serviceability-dev mailing list