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.men...@oracle.com
<mailto:alexey.men...@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
