<div dir="ltr">Hi Alex,<div><br></div><div>Thanks for the review! Yes I had seen that too before sending this review out and forked that fix into this:</div><div> <a href="https://bugs.openjdk.java.net/browse/JDK-8211905" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8211905</a><br></div><div><br></div><div>Which now is merged and I've updated my webrev to reflect this of course.</div><div><br></div><div>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.</div><div><br></div><div>Thanks!</div><div>Jc</div></div><br><div class="gmail_quote"><div dir="ltr">On Wed, Oct 10, 2018 at 5:40 PM Alex Menkov <<a href="mailto:alexey.menkov@oracle.com">alexey.menkov@oracle.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Jc,<br>
<br>
Overall looks good.<br>
<br>
one minor note:<br>
<br>
test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/events/EM06/em06t001/em06t001.cpp:<br>
-    jclassName = (jstring) (jstring) (jstring) (jstring) (jstring) <br>
(jstring) (jstring) (jstring) (jstring) NSK_CPP_STUB3(CallObjectMethod, <br>
jni_env, klass,<br>
-                        methodID);<br>
+    jclassName = (jstring) (jstring) (jstring) (jstring) (jstring) <br>
(jstring) (jstring) (jstring) (jstring) jni_env->CallObjectMethod(klass, <br>
methodID);<br>
<br>
Please drop multi "(jstring)"<br>
<br>
--alex<br>
<br>
On 10/08/2018 21:21, JC Beyler wrote:<br>
> Hi all,<br>
> <br>
> I am continuing the NSK_CPP_STUB removal with this next webrev.<br>
> Webrev: <a href="http://cr.openjdk.java.net/~jcbeyler/8211899/webrev.00/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~jcbeyler/8211899/webrev.00/</a> <br>
> <<a href="http://cr.openjdk.java.net/%7Ejcbeyler/8211899/webrev.00/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8211899/webrev.00/</a>><br>
> Bug: <a href="https://bugs.openjdk.java.net/browse/JDK-8211899" rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8211899</a><br>
> <br>
> The change is still straight-forward though, since it is just doing the <br>
> same NSK_CPP_STUB removal. However when I was looking at the changes, a <br>
> lot of these changes are touching lines with spaces before/after <br>
> parenthesis. I've almost never touched the spaces except if I was <br>
> refactoring by hand the line at the same time. The rationale being that <br>
> the lines will get fixed a few more times and are, at worse, covered by <br>
> the bug: <a href="https://bugs.openjdk.java.net/browse/JDK-8211335" rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8211335</a>, which I've <br>
> commited to doing.<br>
> <br>
> Two exceptions are here where I pushed out the code into assignments due <br>
> to really long lines and complex if structures:<br>
> - jvmti/scenarios/hotswap/HS204/hs204t003/hs204t003.cpp <br>
> <<a href="http://cr.openjdk.java.net/%7Ejcbeyler/8211899/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS204/hs204t003/hs204t003.cpp.udiff.html" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8211899/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS204/hs204t003/hs204t003.cpp.udiff.html</a>><br>
> - jvmti/scenarios/jni_interception/JI01/ji01t001/ji01t001.cpp <br>
> <<a href="http://cr.openjdk.java.net/%7Ejcbeyler/8211899/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/jni_interception/JI01/ji01t001/ji01t001.cpp.udiff.html" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8211899/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/jni_interception/JI01/ji01t001/ji01t001.cpp.udiff.html</a>><br>
> <br>
> And one exception here where a commented line was doing the out-of-if <br>
> assignment so I just uncommented it and used the variable:<br>
> - jvmti/scenarios/hotswap/HS301/hs301t001/hs301t001.cpp <br>
> <<a href="http://cr.openjdk.java.net/%7Ejcbeyler/8211899/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS301/hs301t001/hs301t001.cpp.udiff.html" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8211899/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS301/hs301t001/hs301t001.cpp.udiff.html</a>><br>
> <br>
> I've tested the modified changes on my machine, all modified tests pass.<br>
> <br>
> Let me know what you think,<br>
> Jc<br>
> <br>
> Ps: 2 more of these and we can say good bye to NSK_CPP_STUB*<br>
> <br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><br></div>Thanks,<div>Jc</div></div></div>