Discussion:
RFR (L) 8211899: Remove the NSK_CPP_STUB macros from vmTestbase for jvmti/scenarios/[E-M]
(too old to reply)
JC Beyler
2018-10-09 04:21:30 UTC
Permalink
Hi all,

I am continuing the NSK_CPP_STUB removal with this next webrev.
Webrev: http://cr.openjdk.java.net/~jcbeyler/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/~jcbeyler/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/~jcbeyler/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/~jcbeyler/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*
Alex Menkov
2018-10-11 00:40:03 UTC
Permalink
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
Post by JC Beyler
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/>
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
- 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
- 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*
JC Beyler
2018-10-11 02:03:50 UTC
Permalink
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
Post by Alex Menkov
Hi Jc,
Overall looks good.
- 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
Post by JC Beyler
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/>
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
- 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
Post by JC Beyler
- 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
Post by JC Beyler
And one exception here where a commented line was doing the out-of-if
- 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
Post by JC Beyler
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
Alex Menkov
2018-10-11 18:03:51 UTC
Permalink
got it.

LGTM.

--alex
Post by JC Beyler
Hi Alex,
Thanks for the review! Yes I had seen that too before sending 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
Hi Jc,
Overall looks good.
-    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
Post by JC Beyler
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/>
Post by JC Beyler
<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
Post by JC Beyler
same NSK_CPP_STUB removal. However when I was looking at the
changes, a
Post by JC Beyler
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
Post by JC Beyler
the lines will get fixed a few more times and are, at worse,
covered by
Post by JC Beyler
the bug: https://bugs.openjdk.java.net/browse/JDK-8211335, which
I've
Post by JC Beyler
commited to doing.
Two exceptions are here where I pushed out the code into
assignments due
Post by JC Beyler
- 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>
Post by JC Beyler
- 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>
Post by JC Beyler
And one exception here where a commented line was doing the
out-of-if
Post by JC Beyler
- 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>
Post by JC Beyler
I've tested the modified changes on my machine, all modified
tests pass.
Post by JC Beyler
Let me know what you think,
Jc
Ps: 2 more of these and we can say good bye to NSK_CPP_STUB*
--
Thanks,
Jc
JC Beyler
2018-10-13 00:37:00 UTC
Permalink
Hi all,

Any chance for a second reviewer on a Friday? :)
Jc
Post by Alex Menkov
got it.
LGTM.
--alex
Post by JC Beyler
Hi Alex,
Thanks for the review! Yes I had seen that too before sending 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.
Post by JC Beyler
Thanks!
Jc
Hi Jc,
Overall looks good.
- jclassName = (jstring) (jstring) (jstring) (jstring) (jstring)
(jstring) (jstring) (jstring) (jstring)
NSK_CPP_STUB3(CallObjectMethod,
Post by JC Beyler
jni_env, klass,
- methodID);
+ jclassName = (jstring) (jstring) (jstring) (jstring) (jstring)
(jstring) (jstring) (jstring) (jstring)
jni_env->CallObjectMethod(klass,
methodID);
Please drop multi "(jstring)"
--alex
Post by JC Beyler
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/>
Post by JC Beyler
<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
Post by JC Beyler
same NSK_CPP_STUB removal. However when I was looking at the
changes, a
Post by JC Beyler
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
Post by JC Beyler
the lines will get fixed a few more times and are, at worse,
covered by
Post by JC Beyler
the bug: https://bugs.openjdk.java.net/browse/JDK-8211335, which
I've
Post by JC Beyler
commited to doing.
Two exceptions are here where I pushed out the code into
assignments due
Post by JC Beyler
- 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
Post by JC Beyler
Post by JC Beyler
- 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
Post by JC Beyler
Post by JC Beyler
And one exception here where a commented line was doing the
out-of-if
Post by JC Beyler
- 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
Post by JC Beyler
Post by JC Beyler
I've tested the modified changes on my machine, all modified
tests pass.
Post by JC Beyler
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
JC Beyler
2018-10-16 20:22:40 UTC
Permalink
Hi all,

How about on a Tuesday? :)

Sneak peak and motivational sentence: after this goes in, we have this one
<http://cr.openjdk.java.net/~jcbeyler/8212148/webrev.00/> which removes the
NSK_CPP_STUBs from the tests entirely! :)
Jc
Post by JC Beyler
Hi all,
Any chance for a second reviewer on a Friday? :)
Jc
Post by Alex Menkov
got it.
LGTM.
--alex
Post by JC Beyler
Hi Alex,
Thanks for the review! Yes I had seen that too before sending this
https://bugs.openjdk.java.net/browse/JDK-8211905
Which now is merged and I've updated my webrev to reflect this of
course.
Post by JC Beyler
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.
Post by JC Beyler
Thanks!
Jc
Hi Jc,
Overall looks good.
- jclassName = (jstring) (jstring) (jstring) (jstring) (jstring)
(jstring) (jstring) (jstring) (jstring)
NSK_CPP_STUB3(CallObjectMethod,
Post by JC Beyler
jni_env, klass,
- methodID);
+ jclassName = (jstring) (jstring) (jstring) (jstring) (jstring)
(jstring) (jstring) (jstring) (jstring)
jni_env->CallObjectMethod(klass,
methodID);
Please drop multi "(jstring)"
--alex
Post by JC Beyler
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/>
Post by JC Beyler
<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
Post by JC Beyler
same NSK_CPP_STUB removal. However when I was looking at the
changes, a
Post by JC Beyler
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
Post by JC Beyler
the lines will get fixed a few more times and are, at worse,
covered by
Post by JC Beyler
the bug: https://bugs.openjdk.java.net/browse/JDK-8211335, which
I've
Post by JC Beyler
commited to doing.
Two exceptions are here where I pushed out the code into
assignments due
Post by JC Beyler
- 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
Post by JC Beyler
Post by JC Beyler
- 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
Post by JC Beyler
Post by JC Beyler
And one exception here where a commented line was doing the
out-of-if
Post by JC Beyler
- 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
Post by JC Beyler
Post by JC Beyler
I've tested the modified changes on my machine, all modified
tests pass.
Post by JC Beyler
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
--
Thanks,
Jc
Hohensee, Paul
2018-10-17 20:01:47 UTC
Permalink
Re spaces :). There’s a bunch of places where “(jvmti_env” should be “( jvmti_env”, but of course not all of them. I found a bunch. Otherwise, lgtm., no need for another webrev.

Paul

In hs104t002.cpp

- if (! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities, jvmti,
- &caps) )) {
+ if (! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {

=>

+ if (! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) )) {

In hs203t003.cpp


- if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB4(GetClassSignature,

- jvmti_env, klass, &className, &generic) ) ) {

+ if ( ! NSK_JVMTI_VERIFY (jvmti_env->GetClassSignature(klass, &className, &generic) ) ) {



=>



+ if ( ! NSK_JVMTI_VERIFY ( jvmti_env->GetClassSignature(klass, &className, &generic) ) ) {



and



- if (! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities, jvmti, &caps) )) {

+ if (! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {



=>



+ if (! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) )) {



and



- if ( ! NSK_JVMTI_VERIFY( NSK_CPP_STUB3(GetThreadState, jvmti, thread, &state) ) ) {

+ if ( ! NSK_JVMTI_VERIFY(jvmti->GetThreadState(thread, &state) ) ) {

=>


+ if ( ! NSK_JVMTI_VERIFY( jvmti->GetThreadState(thread, &state) ) ) {

and


- if ( ! NSK_JVMTI_VERIFY( NSK_CPP_STUB3(GetThreadState, jvmti, thread, &state) ) ) {

+ if ( ! NSK_JVMTI_VERIFY(jvmti->GetThreadState(thread, &state) ) ) {

nsk_printf(" Agent :: Error while getting thread state.\n");

nsk_jvmti_agentFailed();

} else {

if ( state & JVMTI_THREAD_STATE_SUSPENDED) {

- if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2( PopFrame, jvmti, thread) ) ) {

+ if ( ! NSK_JVMTI_VERIFY (jvmti->PopFrame(thread) ) ) {

=>


+ if ( ! NSK_JVMTI_VERIFY( jvmti->GetThreadState(thread, &state) ) ) {



+ if ( ! NSK_JVMTI_VERIFY ( jvmti->PopFrame(thread) ) ) {

and


- if ( !NSK_JVMTI_VERIFY( NSK_CPP_STUB2 ( ResumeThread, jvmti, thread)) ) {

+ if ( !NSK_JVMTI_VERIFY(jvmti->ResumeThread(thread)) ) {


=>


+ if ( !NSK_JVMTI_VERIFY( jvmti->ResumeThread(thread)) ) {

In hs203t004.cpp


- if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(GenerateEvents, jvmti_env,

- JVMTI_EVENT_COMPILED_METHOD_LOAD ) )) {

+ if ( ! NSK_JVMTI_VERIFY (jvmti_env->GenerateEvents(JVMTI_EVENT_COMPILED_METHOD_LOAD) )) {

=>


+ if ( ! NSK_JVMTI_VERIFY ( jvmti_env->GenerateEvents(JVMTI_EVENT_COMPILED_METHOD_LOAD) )) {

and


- if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB3(GetMethodDeclaringClass,

- jvmti_env, method, &threadClass) ) ) {

+ if ( ! NSK_JVMTI_VERIFY (jvmti_env->GetMethodDeclaringClass(method, &threadClass) ) ) {

=>


+ if ( ! NSK_JVMTI_VERIFY ( jvmti_env->GetMethodDeclaringClass(method, &threadClass) ) ) {

and


- if (! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities, jvmti, &caps) )) {

+ if (! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {

=>


+ if (! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) )) {

and


- if ( NSK_JVMTI_VERIFY( NSK_CPP_STUB2(SuspendThread, jvmti, thread) ) ) {

+ if ( NSK_JVMTI_VERIFY(jvmti->SuspendThread(thread) ) ) {

=>


+ if ( NSK_JVMTI_VERIFY( jvmti->SuspendThread(thread) ) ) {



and



- if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB3(GetThreadState, jvmti,

- thread, &state) ) ) {

+ if ( ! NSK_JVMTI_VERIFY (jvmti->GetThreadState(thread, &state) ) ) {

NSK_COMPLAIN0("#error Agent :: while getting thread's state.\n");

nsk_jvmti_agentFailed();

} else {

if ( state & JVMTI_THREAD_STATE_SUSPENDED) {

- if ( ! NSK_JVMTI_VERIFY( NSK_CPP_STUB2(PopFrame, jvmti, thread) ) ){

+ if ( ! NSK_JVMTI_VERIFY(jvmti->PopFrame(thread) ) ){



=>



+ if ( ! NSK_JVMTI_VERIFY ( jvmti->GetThreadState(thread, &state) ) ) {




+ if ( ! NSK_JVMTI_VERIFY( jvmti->PopFrame(thread) ) ){


In hs204t001.cpp


- /* if( (myTestClass =NSK_CPP_STUB2(NewGlobalRef,jni_env, klass)) == NULL) {

+ /* if( (myTestClass =jni_env->NewGlobalRef(klass)) == NULL) {

=>


+ /* if( (myTestClass = jni_env->NewGlobalRef(klass)) == NULL) {

and


- if (!NSK_JNI_VERIFY(env, (testClass =(jclass) NSK_CPP_STUB2(NewGlobalRef, env, klass)) != NULL))

+ if (!NSK_JNI_VERIFY(env, (testClass =(jclass) env->NewGlobalRef(klass)) != NULL))

nsk_jvmti_setFailStatus();

- if (!NSK_JNI_VERIFY(env, (testedThread =NSK_CPP_STUB2(NewGlobalRef, env, thread)) != NULL))

+ if (!NSK_JNI_VERIFY(env, (testedThread =env->NewGlobalRef(thread)) != NULL))

=>


+ if (!NSK_JNI_VERIFY(env, (testClass = (jclass) env->NewGlobalRef(klass)) != NULL))

nsk_jvmti_setFailStatus();




+ if (!NSK_JNI_VERIFY(env, (testedThread = env->NewGlobalRef(thread)) != NULL))

and


- if (!NSK_JVMTI_VERIFY( NSK_CPP_STUB2(SuspendThread, jvmti, thread))) {

+ if (!NSK_JVMTI_VERIFY( jvmti->SuspendThread(thread))) {

=>


+ if (!NSK_JVMTI_VERIFY(jvmti->SuspendThread(thread))) {

In hs204t003.cpp


- if (! NSK_JVMTI_VERIFY( NSK_CPP_STUB3(GetThreadState, jvmti, thread, &state)) ){

+ if (! NSK_JVMTI_VERIFY(jvmti->GetThreadState(thread, &state)) ){

NSK_DISPLAY0(" Agent :: Error getting thread state.\n");

nsk_jvmti_agentFailed();

} else {

if ( state & JVMTI_THREAD_STATE_SUSPENDED) {

NSK_DISPLAY0(" Agent :: Thread state = JVMTI_THREAD_STATE_SUSPENDED.\n");

- if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(PopFrame, jvmti, thread) ) ) {

+ if ( ! NSK_JVMTI_VERIFY (jvmti->PopFrame(thread) ) ) {

NSK_DISPLAY0("#error Agent :: Jvmti failed to do popFrame.\n");

nsk_jvmti_agentFailed();

} else {

- if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(ResumeThread, jvmti, thread)) ) {

+ if ( ! NSK_JVMTI_VERIFY (jvmti->ResumeThread(thread)) ) {

=>


+ if (! NSK_JVMTI_VERIFY( jvmti->GetThreadState(thread, &state)) ){




+ if ( ! NSK_JVMTI_VERIFY ( jvmti->PopFrame(thread) ) ) {




+ if ( ! NSK_JVMTI_VERIFY ( jvmti->ResumeThread(thread)) ) {

In hs301t001.cpp


- if (! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities, jvmti, &caps) )) {

+ if (! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {

=>


+ if (! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) )) {

In hs301t002.cpp


- if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities, jvmti, &caps) ) ) {

+ if ( ! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) ) ) {

=>


+ if ( ! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) ) ) {

In hs301t003.cpp


- if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB4(GetClassSignature,

- jvmti_env, klass, &className, &generic) ) ) {

+ if ( ! NSK_JVMTI_VERIFY (jvmti_env->GetClassSignature(klass, &className, &generic) ) ) {

=>


+ if ( ! NSK_JVMTI_VERIFY ( jvmti_env->GetClassSignature(klass, &className, &generic) ) ) {

and


- if (! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities, jvmti, &caps) )) {

+ if (! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {

=>


+ if (! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) )) {

and


- if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities,

- jvmti, &caps) )) {

+ if ( ! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {

=>


+ if ( ! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) )) {

From: serviceability-dev <serviceability-dev-***@openjdk.java.net> on behalf of JC Beyler <***@google.com>
Date: Tuesday, October 16, 2018 at 4:24 PM
To: "***@oracle.com" <***@oracle.com>
Cc: "serviceability-***@openjdk.java.net" <serviceability-***@openjdk.java.net>
Subject: Re: RFR (L) 8211899: Remove the NSK_CPP_STUB macros from vmTestbase for jvmti/scenarios/[E-M]

Hi all,

How about on a Tuesday? :)

Sneak peak and motivational sentence: after this goes in, we have this one<http://cr.openjdk.java.net/~jcbeyler/8212148/webrev.00/> which removes the NSK_CPP_STUBs from the tests entirely! :)
Jc


On Fri, Oct 12, 2018 at 5:37 PM JC Beyler <***@google.com<mailto:***@google.com>> wrote:
Hi all,

Any chance for a second reviewer on a Friday? :)
Jc

On Thu, Oct 11, 2018 at 11:03 AM Alex Menkov <***@oracle.com<mailto:***@oracle.com>> wrote:
got it.

LGTM.

--alex
Post by JC Beyler
Hi Alex,
Thanks for the review! Yes I had seen that too before sending 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
Hi Jc,
Overall looks good.
- 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
Post by JC Beyler
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/>
Post by JC Beyler
<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
Post by JC Beyler
same NSK_CPP_STUB removal. However when I was looking at the
changes, a
Post by JC Beyler
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
Post by JC Beyler
the lines will get fixed a few more times and are, at worse,
covered by
Post by JC Beyler
the bug: https://bugs.openjdk.java.net/browse/JDK-8211335, which
I've
Post by JC Beyler
commited to doing.
Two exceptions are here where I pushed out the code into
assignments due
Post by JC Beyler
- 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>
Post by JC Beyler
- 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>
Post by JC Beyler
And one exception here where a commented line was doing the
out-of-if
Post by JC Beyler
- 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>
Post by JC Beyler
I've tested the modified changes on my machine, all modified
tests pass.
Post by JC Beyler
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
--
Thanks,
Jc
JC Beyler
2018-10-17 22:18:47 UTC
Permalink
Hi Paul,

Thanks for the review :)

I've fixed the cases you saw to at least keep them consistent. However, I'm
not too worried about these as my next set of webrevs is going to fix the
spaces for all ( and ) in the vmTestbase so we will fix everything once and
for all.

The JDK bug I'm referring to is :
https://bugs.openjdk.java.net/browse/JDK-8211335

I already have relatively the scripts to do it :-),
Jc
Post by Hohensee, Paul
Re spaces :). There’s a bunch of places where “(jvmti_env” should be “(
jvmti_env”, but of course not all of them. I found a bunch. Otherwise,
lgtm., no need for another webrev.
Paul
In hs104t002.cpp
- if (! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities, jvmti,
- &caps) )) {
+ if (! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {
=>
+ if (! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) )) {
In hs203t003.cpp
- if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB4(GetClassSignature,
- jvmti_env, klass, &className, &generic) ) ) {
+ if ( ! NSK_JVMTI_VERIFY (jvmti_env->GetClassSignature(klass, &className, &generic) ) ) {
=>
+ if ( ! NSK_JVMTI_VERIFY ( jvmti_env->GetClassSignature(klass, &className, &generic) ) ) {
and
- if (! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities, jvmti, &caps) )) {
+ if (! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {
=>
+ if (! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) )) {
and
- if ( ! NSK_JVMTI_VERIFY( NSK_CPP_STUB3(GetThreadState, jvmti, thread, &state) ) ) {
+ if ( ! NSK_JVMTI_VERIFY(jvmti->GetThreadState(thread, &state) ) ) {
=>
+ if ( ! NSK_JVMTI_VERIFY( jvmti->GetThreadState(thread, &state) ) ) {
and
- if ( ! NSK_JVMTI_VERIFY( NSK_CPP_STUB3(GetThreadState, jvmti, thread, &state) ) ) {
+ if ( ! NSK_JVMTI_VERIFY(jvmti->GetThreadState(thread, &state) ) ) {
nsk_printf(" Agent :: Error while getting thread state.\n");
nsk_jvmti_agentFailed();
} else {
if ( state & JVMTI_THREAD_STATE_SUSPENDED) {
- if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2( PopFrame, jvmti, thread) ) ) {
+ if ( ! NSK_JVMTI_VERIFY (jvmti->PopFrame(thread) ) ) {
=>
+ if ( ! NSK_JVMTI_VERIFY( jvmti->GetThreadState(thread, &state) ) ) {


+ if ( ! NSK_JVMTI_VERIFY ( jvmti->PopFrame(thread) ) ) {
and
- if ( !NSK_JVMTI_VERIFY( NSK_CPP_STUB2 ( ResumeThread, jvmti, thread)) ) {
+ if ( !NSK_JVMTI_VERIFY(jvmti->ResumeThread(thread)) ) {
=>
+ if ( !NSK_JVMTI_VERIFY( jvmti->ResumeThread(thread)) ) {
In hs203t004.cpp
- if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(GenerateEvents, jvmti_env,
- JVMTI_EVENT_COMPILED_METHOD_LOAD ) )) {
+ if ( ! NSK_JVMTI_VERIFY (jvmti_env->GenerateEvents(JVMTI_EVENT_COMPILED_METHOD_LOAD) )) {
=>
+ if ( ! NSK_JVMTI_VERIFY ( jvmti_env->GenerateEvents(JVMTI_EVENT_COMPILED_METHOD_LOAD) )) {
and
- if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB3(GetMethodDeclaringClass,
- jvmti_env, method, &threadClass) ) ) {
+ if ( ! NSK_JVMTI_VERIFY (jvmti_env->GetMethodDeclaringClass(method, &threadClass) ) ) {
=>
+ if ( ! NSK_JVMTI_VERIFY ( jvmti_env->GetMethodDeclaringClass(method, &threadClass) ) ) {
and
- if (! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities, jvmti, &caps) )) {
+ if (! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {
=>
+ if (! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) )) {
and
- if ( NSK_JVMTI_VERIFY( NSK_CPP_STUB2(SuspendThread, jvmti, thread) ) ) {
+ if ( NSK_JVMTI_VERIFY(jvmti->SuspendThread(thread) ) ) {
=>
+ if ( NSK_JVMTI_VERIFY( jvmti->SuspendThread(thread) ) ) {
and
- if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB3(GetThreadState, jvmti,
- thread, &state) ) ) {
+ if ( ! NSK_JVMTI_VERIFY (jvmti->GetThreadState(thread, &state) ) ) {
NSK_COMPLAIN0("#error Agent :: while getting thread's state.\n");
nsk_jvmti_agentFailed();
} else {
if ( state & JVMTI_THREAD_STATE_SUSPENDED) {
- if ( ! NSK_JVMTI_VERIFY( NSK_CPP_STUB2(PopFrame, jvmti, thread) ) ){
+ if ( ! NSK_JVMTI_VERIFY(jvmti->PopFrame(thread) ) ){
=>
+ if ( ! NSK_JVMTI_VERIFY ( jvmti->GetThreadState(thread, &state) ) ) {


+ if ( ! NSK_JVMTI_VERIFY( jvmti->PopFrame(thread) ) ){
In hs204t001.cpp
- /* if( (myTestClass =NSK_CPP_STUB2(NewGlobalRef,jni_env, klass)) == NULL) {
+ /* if( (myTestClass =jni_env->NewGlobalRef(klass)) == NULL) {
=>
+ /* if( (myTestClass = jni_env->NewGlobalRef(klass)) == NULL) {
and
- if (!NSK_JNI_VERIFY(env, (testClass =(jclass) NSK_CPP_STUB2(NewGlobalRef, env, klass)) != NULL))
+ if (!NSK_JNI_VERIFY(env, (testClass =(jclass) env->NewGlobalRef(klass)) != NULL))
nsk_jvmti_setFailStatus();
- if (!NSK_JNI_VERIFY(env, (testedThread =NSK_CPP_STUB2(NewGlobalRef, env, thread)) != NULL))
+ if (!NSK_JNI_VERIFY(env, (testedThread =env->NewGlobalRef(thread)) != NULL))
=>
+ if (!NSK_JNI_VERIFY(env, (testClass = (jclass) env->NewGlobalRef(klass)) != NULL))
nsk_jvmti_setFailStatus();


+ if (!NSK_JNI_VERIFY(env, (testedThread = env->NewGlobalRef(thread)) != NULL))
and
- if (!NSK_JVMTI_VERIFY( NSK_CPP_STUB2(SuspendThread, jvmti, thread))) {
+ if (!NSK_JVMTI_VERIFY( jvmti->SuspendThread(thread))) {
=>
+ if (!NSK_JVMTI_VERIFY(jvmti->SuspendThread(thread))) {
In hs204t003.cpp
- if (! NSK_JVMTI_VERIFY( NSK_CPP_STUB3(GetThreadState, jvmti, thread, &state)) ){
+ if (! NSK_JVMTI_VERIFY(jvmti->GetThreadState(thread, &state)) ){
NSK_DISPLAY0(" Agent :: Error getting thread state.\n");
nsk_jvmti_agentFailed();
} else {
if ( state & JVMTI_THREAD_STATE_SUSPENDED) {
NSK_DISPLAY0(" Agent :: Thread state = JVMTI_THREAD_STATE_SUSPENDED.\n");
- if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(PopFrame, jvmti, thread) ) ) {
+ if ( ! NSK_JVMTI_VERIFY (jvmti->PopFrame(thread) ) ) {
NSK_DISPLAY0("#error Agent :: Jvmti failed to do popFrame.\n");
nsk_jvmti_agentFailed();
} else {
- if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(ResumeThread, jvmti, thread)) ) {
+ if ( ! NSK_JVMTI_VERIFY (jvmti->ResumeThread(thread)) ) {
=>
+ if (! NSK_JVMTI_VERIFY( jvmti->GetThreadState(thread, &state)) ){


+ if ( ! NSK_JVMTI_VERIFY ( jvmti->PopFrame(thread) ) ) {


+ if ( ! NSK_JVMTI_VERIFY ( jvmti->ResumeThread(thread)) ) {
In hs301t001.cpp
- if (! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities, jvmti, &caps) )) {
+ if (! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {
=>
+ if (! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) )) {
In hs301t002.cpp
- if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities, jvmti, &caps) ) ) {
+ if ( ! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) ) ) {
=>
+ if ( ! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) ) ) {
In hs301t003.cpp
- if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB4(GetClassSignature,
- jvmti_env, klass, &className, &generic) ) ) {
+ if ( ! NSK_JVMTI_VERIFY (jvmti_env->GetClassSignature(klass, &className, &generic) ) ) {
=>
+ if ( ! NSK_JVMTI_VERIFY ( jvmti_env->GetClassSignature(klass, &className, &generic) ) ) {
and
- if (! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities, jvmti, &caps) )) {
+ if (! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {
=>
+ if (! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) )) {
and
- if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities,
- jvmti, &caps) )) {
+ if ( ! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {
=>
+ if ( ! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) )) {
*Date: *Tuesday, October 16, 2018 at 4:24 PM
*Subject: *Re: RFR (L) 8211899: Remove the NSK_CPP_STUB macros from
vmTestbase for jvmti/scenarios/[E-M]
Hi all,
How about on a Tuesday? :)
Sneak peak and motivational sentence: after this goes in, we have this one
<http://cr.openjdk.java.net/~jcbeyler/8212148/webrev.00/> which removes
the NSK_CPP_STUBs from the tests entirely! :)
Jc
Hi all,
Any chance for a second reviewer on a Friday? :)
Jc
got it.
LGTM.
--alex
Post by JC Beyler
Hi Alex,
Thanks for the review! Yes I had seen that too before sending 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.
Post by JC Beyler
Thanks!
Jc
Hi Jc,
Overall looks good.
- jclassName = (jstring) (jstring) (jstring) (jstring) (jstring)
(jstring) (jstring) (jstring) (jstring)
NSK_CPP_STUB3(CallObjectMethod,
Post by JC Beyler
jni_env, klass,
- methodID);
+ jclassName = (jstring) (jstring) (jstring) (jstring) (jstring)
(jstring) (jstring) (jstring) (jstring)
jni_env->CallObjectMethod(klass,
methodID);
Please drop multi "(jstring)"
--alex
Post by JC Beyler
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/>
Post by JC Beyler
<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
Post by JC Beyler
same NSK_CPP_STUB removal. However when I was looking at the
changes, a
Post by JC Beyler
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
Post by JC Beyler
the lines will get fixed a few more times and are, at worse,
covered by
Post by JC Beyler
the bug: https://bugs.openjdk.java.net/browse/JDK-8211335, which
I've
Post by JC Beyler
commited to doing.
Two exceptions are here where I pushed out the code into
assignments due
Post by JC Beyler
- 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
Post by JC Beyler
Post by JC Beyler
- 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
Post by JC Beyler
Post by JC Beyler
And one exception here where a commented line was doing the
out-of-if
Post by JC Beyler
- 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
Post by JC Beyler
Post by JC Beyler
I've tested the modified changes on my machine, all modified
tests pass.
Post by JC Beyler
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
--
Thanks,
Jc
--
Thanks,
Jc
David Holmes
2018-10-18 01:34:01 UTC
Permalink
Re spaces :).  There’s a bunch of places where “(jvmti_env”  should be
“( jvmti_env”, but of course not all of them. I found a bunch.
Why should there be a space after the '(' ? That's not hotspot-style.
Neither is a space before ')'.

David
Otherwise, lgtm., no need for another webrev.
Paul
In hs104t002.cpp
-        if (! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities, jvmti,
-                &caps) )) {
+        if (! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {
=>
+        if (! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) )) {
In hs203t003.cpp
-    if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB4(GetClassSignature,
-                    jvmti_env, klass, &className, &generic) ) ) {
+    if ( ! NSK_JVMTI_VERIFY (jvmti_env->GetClassSignature(klass,
&className, &generic) ) ) {
=>
+    if ( ! NSK_JVMTI_VERIFY ( jvmti_env->GetClassSignature(klass,
&className, &generic) ) ) {
and
-        if (! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities, jvmti,
&caps) )) {
+        if (! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {
=>
+        if (! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) )) {
and
-    if ( ! NSK_JVMTI_VERIFY( NSK_CPP_STUB3(GetThreadState, jvmti,
thread, &state) )  ) {
+    if ( ! NSK_JVMTI_VERIFY(jvmti->GetThreadState(thread, &state) )  ) {
=>
+    if ( ! NSK_JVMTI_VERIFY( jvmti->GetThreadState(thread, &state) )  ) {
and
-    if ( ! NSK_JVMTI_VERIFY( NSK_CPP_STUB3(GetThreadState, jvmti,
thread, &state) )  ) {
+    if ( ! NSK_JVMTI_VERIFY(jvmti->GetThreadState(thread, &state) )  ) {
         nsk_printf(" Agent :: Error while getting thread state.\n");
         nsk_jvmti_agentFailed();
     } else {
         if ( state & JVMTI_THREAD_STATE_SUSPENDED) {
-            if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2( PopFrame, jvmti,
thread) ) ) {
+            if ( ! NSK_JVMTI_VERIFY (jvmti->PopFrame(thread) ) ) {
=>
+    if ( ! NSK_JVMTI_VERIFY( jvmti->GetThreadState(thread, &state) )  ) {

+            if ( ! NSK_JVMTI_VERIFY ( jvmti->PopFrame(thread) ) ) {
and
-    if ( !NSK_JVMTI_VERIFY( NSK_CPP_STUB2 ( ResumeThread, jvmti,
thread)) ) {
+    if ( !NSK_JVMTI_VERIFY(jvmti->ResumeThread(thread)) ) {
=>
+    if ( !NSK_JVMTI_VERIFY( jvmti->ResumeThread(thread)) ) {
In hs203t004.cpp
-                if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(GenerateEvents,
jvmti_env,
-                                JVMTI_EVENT_COMPILED_METHOD_LOAD ) )) {
+                if ( ! NSK_JVMTI_VERIFY
(jvmti_env->GenerateEvents(JVMTI_EVENT_COMPILED_METHOD_LOAD) )) {
=>
+                if ( ! NSK_JVMTI_VERIFY (
jvmti_env->GenerateEvents(JVMTI_EVENT_COMPILED_METHOD_LOAD) )) {
and
-        if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB3(GetMethodDeclaringClass,
-                        jvmti_env, method, &threadClass) ) ) {
+        if ( ! NSK_JVMTI_VERIFY
(jvmti_env->GetMethodDeclaringClass(method, &threadClass) ) ) {
=>
+        if ( ! NSK_JVMTI_VERIFY (
jvmti_env->GetMethodDeclaringClass(method, &threadClass) ) ) {
and
-        if (! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities, jvmti,
&caps) )) {
+        if (! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {
=>
+        if (! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) )) {
and
-    if (  NSK_JVMTI_VERIFY( NSK_CPP_STUB2(SuspendThread, jvmti, thread)
) ) {
+    if (  NSK_JVMTI_VERIFY(jvmti->SuspendThread(thread) ) ) {
=>
+    if (  NSK_JVMTI_VERIFY( jvmti->SuspendThread(thread) ) ) {
and
-    if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB3(GetThreadState, jvmti,
-                    thread, &state) ) ) {
+    if ( ! NSK_JVMTI_VERIFY (jvmti->GetThreadState(thread, &state) ) ) {
         NSK_COMPLAIN0("#error Agent :: while getting thread's state.\n");
         nsk_jvmti_agentFailed();
     } else {
         if ( state & JVMTI_THREAD_STATE_SUSPENDED) {
-            if ( ! NSK_JVMTI_VERIFY( NSK_CPP_STUB2(PopFrame, jvmti,
thread) ) ){
+            if ( ! NSK_JVMTI_VERIFY(jvmti->PopFrame(thread) ) ){
=>
+    if ( ! NSK_JVMTI_VERIFY ( jvmti->GetThreadState(thread, &state) ) ) {

+            if ( ! NSK_JVMTI_VERIFY( jvmti->PopFrame(thread) ) ){
In hs204t001.cpp
-        /* if( (myTestClass =NSK_CPP_STUB2(NewGlobalRef,jni_env,
klass)) == NULL) {
+        /* if( (myTestClass =jni_env->NewGlobalRef(klass)) == NULL) {
=>
+        /* if( (myTestClass = jni_env->NewGlobalRef(klass)) == NULL) {
and
-    if (!NSK_JNI_VERIFY(env, (testClass =(jclass)
NSK_CPP_STUB2(NewGlobalRef, env, klass)) != NULL))
+    if (!NSK_JNI_VERIFY(env, (testClass =(jclass)
env->NewGlobalRef(klass)) != NULL))
         nsk_jvmti_setFailStatus();
-    if (!NSK_JNI_VERIFY(env, (testedThread =NSK_CPP_STUB2(NewGlobalRef,
env, thread)) != NULL))
+    if (!NSK_JNI_VERIFY(env, (testedThread =env->NewGlobalRef(thread))
!= NULL))
=>
+    if (!NSK_JNI_VERIFY(env, (testClass = (jclass)
env->NewGlobalRef(klass)) != NULL))
         nsk_jvmti_setFailStatus();

+    if (!NSK_JNI_VERIFY(env, (testedThread = env->NewGlobalRef(thread))
!= NULL))
and
-            if (!NSK_JVMTI_VERIFY(  NSK_CPP_STUB2(SuspendThread, jvmti,
thread))) {
+            if (!NSK_JVMTI_VERIFY(  jvmti->SuspendThread(thread))) {
=>
+            if (!NSK_JVMTI_VERIFY(jvmti->SuspendThread(thread))) {
In hs204t003.cpp
-    if (! NSK_JVMTI_VERIFY( NSK_CPP_STUB3(GetThreadState, jvmti,
thread, &state)) ){
+    if (! NSK_JVMTI_VERIFY(jvmti->GetThreadState(thread, &state)) ){
         NSK_DISPLAY0(" Agent :: Error getting thread state.\n");
         nsk_jvmti_agentFailed();
     } else {
         if ( state & JVMTI_THREAD_STATE_SUSPENDED) {
             NSK_DISPLAY0(" Agent :: Thread state = JVMTI_THREAD_STATE_SUSPENDED.\n");
-            if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(PopFrame, jvmti,
thread) ) ) {
+            if ( ! NSK_JVMTI_VERIFY (jvmti->PopFrame(thread) ) ) {
                 NSK_DISPLAY0("#error Agent :: Jvmti failed to do popFrame.\n");
                 nsk_jvmti_agentFailed();
             } else {
-                if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(ResumeThread,
jvmti, thread)) ) {
+                if ( ! NSK_JVMTI_VERIFY (jvmti->ResumeThread(thread)) ) {
=>
+    if (! NSK_JVMTI_VERIFY( jvmti->GetThreadState(thread, &state)) ){

+            if ( ! NSK_JVMTI_VERIFY ( jvmti->PopFrame(thread) ) ) {

+                if ( ! NSK_JVMTI_VERIFY ( jvmti->ResumeThread(thread)) ) {
In hs301t001.cpp
-        if (! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities, jvmti,
&caps) )) {
+        if (! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {
=>
+        if (! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) )) {
In hs301t002.cpp
-        if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities, jvmti,
&caps) ) ) {
+        if ( ! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) ) ) {
=>
+        if ( ! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) ) ) {
In hs301t003.cpp
-  if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB4(GetClassSignature,
-                  jvmti_env, klass, &className, &generic) ) ) {
+  if ( ! NSK_JVMTI_VERIFY (jvmti_env->GetClassSignature(klass,
&className, &generic) ) ) {
=>
+  if ( ! NSK_JVMTI_VERIFY ( jvmti_env->GetClassSignature(klass,
&className, &generic) ) ) {
and
-        if (! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities, jvmti,
&caps) ))  {
+        if (! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) ))  {
=>
+        if (! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) ))  {
and
-        if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities,
-                        jvmti, &caps) ))  {
+        if ( ! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) ))  {
=>
+        if ( ! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) ))  {
*Date: *Tuesday, October 16, 2018 at 4:24 PM
*Subject: *Re: RFR (L) 8211899: Remove the NSK_CPP_STUB macros from
vmTestbase for jvmti/scenarios/[E-M]
Hi all,
How about on a Tuesday? :)
Sneak peak and motivational sentence: after this goes in, we have this
one <http://cr.openjdk.java.net/%7Ejcbeyler/8212148/webrev.00/> which
removes the NSK_CPP_STUBs from the tests entirely! :)
Jc
Hi all,
Any chance for a second reviewer on a Friday? :)
Jc
On Thu, Oct 11, 2018 at 11:03 AM Alex Menkov
got it.
LGTM.
--alex
Post by JC Beyler
Hi Alex,
Thanks for the review! Yes I had seen that too before sending
this
Post by JC Beyler
https://bugs.openjdk.java.net/browse/JDK-8211905
Which now is merged and I've updated my webrev to reflect
this of course.
Post by JC Beyler
My rationale for not doing it here directly is always the
same: keeping
Post by JC Beyler
the changes to the "spirit" of what the change is trying to
do. Those
Post by JC Beyler
extra casts were a bit out-of-scope and so I just forked the
fix in 8211905.
Post by JC Beyler
Thanks!
Jc
On Wed, Oct 10, 2018 at 5:40 PM Alex Menkov
     Hi Jc,
     Overall looks good.
     -    jclassName = (jstring) (jstring) (jstring) (jstring)
(jstring)
Post by JC Beyler
     (jstring) (jstring) (jstring) (jstring)
NSK_CPP_STUB3(CallObjectMethod,
Post by JC Beyler
     jni_env, klass,
     -                        methodID);
     +    jclassName = (jstring) (jstring) (jstring) (jstring)
(jstring)
Post by JC Beyler
     (jstring) (jstring) (jstring) (jstring)
     jni_env->CallObjectMethod(klass,
     methodID);
     Please drop multi "(jstring)"
     --alex
      > Hi all,
      >
      > I am continuing the NSK_CPP_STUB removal with this
next webrev.
http://cr.openjdk.java.net/~jcbeyler/8211899/webrev.00/
<http://cr.openjdk.java.net/%7Ejcbeyler/8211899/webrev.00/>
Post by JC Beyler
     <http://cr.openjdk.java.net/%7Ejcbeyler/8211899/webrev.00/>
      >
<http://cr.openjdk.java.net/%7Ejcbeyler/8211899/webrev.00/>
Post by JC Beyler
      > Bug: https://bugs.openjdk.java.net/browse/JDK-8211899
      >
      > The change is still straight-forward though, since it
is just
Post by JC Beyler
     doing the
      > same NSK_CPP_STUB removal. However when I was looking
at the
Post by JC Beyler
     changes, a
      > lot of these changes are touching lines with spaces
before/after
Post by JC Beyler
      > parenthesis. I've almost never touched the spaces
except if I was
Post by JC Beyler
      > refactoring by hand the line at the same time. The
rationale
Post by JC Beyler
     being that
      > the lines will get fixed a few more times and are, at
worse,
Post by JC Beyler
     covered by
https://bugs.openjdk.java.net/browse/JDK-8211335, which
Post by JC Beyler
     I've
      > commited to doing.
      >
      > Two exceptions are here where I pushed out the code into
     assignments due
      > - 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>
Post by JC Beyler
      > -
jvmti/scenarios/jni_interception/JI01/ji01t001/ji01t001.cpp
Post by JC Beyler
      >
 <http://cr.openjdk.java.net/%7Ejcbeyler/8211899/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/jni_interception/JI01/ji01t001/ji01t001.cpp.udiff.html>
Post by JC Beyler
      >
      > And one exception here where a commented line was
doing the
Post by JC Beyler
     out-of-if
      > - 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>
Post by JC Beyler
      >
      > I've tested the modified changes on my machine, all
modified
Post by JC Beyler
     tests pass.
      >
      > Let me know what you think,
      > Jc
      >
      > Ps: 2 more of these and we can say good bye to
NSK_CPP_STUB*
Post by JC Beyler
      >
--
Thanks,
Jc
--
Thanks,
Jc
--
JC Beyler
2018-10-18 01:57:38 UTC
Permalink
Hi all,

@Serguei: no worries for the time taken, I am just two webrevs away from
conquering the NSK_CPP_STUBs, I might have been a bit pushy for a review
:-) I apologize!

@David: It's not hotspot style but a lot of those files had that style. My
CL actually makes it non-conforming all around:

if ( before_we_had_this ) // space after ( + space before )

if (now_we_have_this ) // only space before )

which is kind of weird and Paul's comment was to at least try to not make
it inconsistent to that point.

I have one webrev to go that will remove NSK_CPP_STUBs and then I'm going
to fix all the spaces (via https://bugs.openjdk.java.net/browse/JDK-8211335)
for these .cpp files so we won't have the issue anymore.

Does that make sense?

Thanks!
Jc
Post by David Holmes
Post by Hohensee, Paul
Re spaces :). There’s a bunch of places where “(jvmti_env” should be
“( jvmti_env”, but of course not all of them. I found a bunch.
Why should there be a space after the '(' ? That's not hotspot-style.
Neither is a space before ')'.
David
Post by Hohensee, Paul
Otherwise, lgtm., no need for another webrev.
Paul
In hs104t002.cpp
- if (! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities, jvmti,
- &caps) )) {
+ if (! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {
=>
+ if (! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) )) {
In hs203t003.cpp
- if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB4(GetClassSignature,
- jvmti_env, klass, &className, &generic) ) ) {
+ if ( ! NSK_JVMTI_VERIFY (jvmti_env->GetClassSignature(klass,
&className, &generic) ) ) {
=>
+ if ( ! NSK_JVMTI_VERIFY ( jvmti_env->GetClassSignature(klass,
&className, &generic) ) ) {
and
- if (! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities, jvmti, &caps) )) {
+ if (! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {
=>
+ if (! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) )) {
and
- if ( ! NSK_JVMTI_VERIFY( NSK_CPP_STUB3(GetThreadState, jvmti,
thread, &state) ) ) {
+ if ( ! NSK_JVMTI_VERIFY(jvmti->GetThreadState(thread, &state) ) ) {
=>
+ if ( ! NSK_JVMTI_VERIFY( jvmti->GetThreadState(thread, &state) ) )
{
Post by Hohensee, Paul
and
- if ( ! NSK_JVMTI_VERIFY( NSK_CPP_STUB3(GetThreadState, jvmti,
thread, &state) ) ) {
+ if ( ! NSK_JVMTI_VERIFY(jvmti->GetThreadState(thread, &state) ) ) {
nsk_printf(" Agent :: Error while getting thread state.\n");
nsk_jvmti_agentFailed();
} else {
if ( state & JVMTI_THREAD_STATE_SUSPENDED) {
- if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2( PopFrame, jvmti, thread) ) ) {
+ if ( ! NSK_JVMTI_VERIFY (jvmti->PopFrame(thread) ) ) {
=>
+ if ( ! NSK_JVMTI_VERIFY( jvmti->GetThreadState(thread, &state) ) )
{
Post by Hohensee, Paul


+ if ( ! NSK_JVMTI_VERIFY ( jvmti->PopFrame(thread) ) ) {
and
- if ( !NSK_JVMTI_VERIFY( NSK_CPP_STUB2 ( ResumeThread, jvmti, thread)) ) {
+ if ( !NSK_JVMTI_VERIFY(jvmti->ResumeThread(thread)) ) {
=>
+ if ( !NSK_JVMTI_VERIFY( jvmti->ResumeThread(thread)) ) {
In hs203t004.cpp
- if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(GenerateEvents, jvmti_env,
- JVMTI_EVENT_COMPILED_METHOD_LOAD ) )) {
+ if ( ! NSK_JVMTI_VERIFY
(jvmti_env->GenerateEvents(JVMTI_EVENT_COMPILED_METHOD_LOAD) )) {
=>
+ if ( ! NSK_JVMTI_VERIFY (
jvmti_env->GenerateEvents(JVMTI_EVENT_COMPILED_METHOD_LOAD) )) {
and
- if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB3(GetMethodDeclaringClass,
- jvmti_env, method, &threadClass) ) ) {
+ if ( ! NSK_JVMTI_VERIFY
(jvmti_env->GetMethodDeclaringClass(method, &threadClass) ) ) {
=>
+ if ( ! NSK_JVMTI_VERIFY (
jvmti_env->GetMethodDeclaringClass(method, &threadClass) ) ) {
and
- if (! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities, jvmti, &caps) )) {
+ if (! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {
=>
+ if (! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) )) {
and
- if ( NSK_JVMTI_VERIFY( NSK_CPP_STUB2(SuspendThread, jvmti, thread) ) ) {
+ if ( NSK_JVMTI_VERIFY(jvmti->SuspendThread(thread) ) ) {
=>
+ if ( NSK_JVMTI_VERIFY( jvmti->SuspendThread(thread) ) ) {
and
- if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB3(GetThreadState, jvmti,
- thread, &state) ) ) {
+ if ( ! NSK_JVMTI_VERIFY (jvmti->GetThreadState(thread, &state) ) ) {
NSK_COMPLAIN0("#error Agent :: while getting thread's
state.\n");
Post by Hohensee, Paul
nsk_jvmti_agentFailed();
} else {
if ( state & JVMTI_THREAD_STATE_SUSPENDED) {
- if ( ! NSK_JVMTI_VERIFY( NSK_CPP_STUB2(PopFrame, jvmti, thread) ) ){
+ if ( ! NSK_JVMTI_VERIFY(jvmti->PopFrame(thread) ) ){
=>
+ if ( ! NSK_JVMTI_VERIFY ( jvmti->GetThreadState(thread, &state) ) )
{
Post by Hohensee, Paul


+ if ( ! NSK_JVMTI_VERIFY( jvmti->PopFrame(thread) ) ){
In hs204t001.cpp
- /* if( (myTestClass =NSK_CPP_STUB2(NewGlobalRef,jni_env, klass)) == NULL) {
+ /* if( (myTestClass =jni_env->NewGlobalRef(klass)) == NULL) {
=>
+ /* if( (myTestClass = jni_env->NewGlobalRef(klass)) == NULL) {
and
- if (!NSK_JNI_VERIFY(env, (testClass =(jclass)
NSK_CPP_STUB2(NewGlobalRef, env, klass)) != NULL))
+ if (!NSK_JNI_VERIFY(env, (testClass =(jclass)
env->NewGlobalRef(klass)) != NULL))
nsk_jvmti_setFailStatus();
- if (!NSK_JNI_VERIFY(env, (testedThread =NSK_CPP_STUB2(NewGlobalRef,
env, thread)) != NULL))
+ if (!NSK_JNI_VERIFY(env, (testedThread =env->NewGlobalRef(thread)) != NULL))
=>
+ if (!NSK_JNI_VERIFY(env, (testClass = (jclass)
env->NewGlobalRef(klass)) != NULL))
nsk_jvmti_setFailStatus();


+ if (!NSK_JNI_VERIFY(env, (testedThread = env->NewGlobalRef(thread)) != NULL))
and
- if (!NSK_JVMTI_VERIFY( NSK_CPP_STUB2(SuspendThread, jvmti, thread))) {
+ if (!NSK_JVMTI_VERIFY( jvmti->SuspendThread(thread))) {
=>
+ if (!NSK_JVMTI_VERIFY(jvmti->SuspendThread(thread))) {
In hs204t003.cpp
- if (! NSK_JVMTI_VERIFY( NSK_CPP_STUB3(GetThreadState, jvmti, thread, &state)) ){
+ if (! NSK_JVMTI_VERIFY(jvmti->GetThreadState(thread, &state)) ){
NSK_DISPLAY0(" Agent :: Error getting thread state.\n");
nsk_jvmti_agentFailed();
} else {
if ( state & JVMTI_THREAD_STATE_SUSPENDED) {
NSK_DISPLAY0(" Agent :: Thread state =
JVMTI_THREAD_STATE_SUSPENDED.\n");
Post by Hohensee, Paul
- if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(PopFrame, jvmti, thread) ) ) {
+ if ( ! NSK_JVMTI_VERIFY (jvmti->PopFrame(thread) ) ) {
NSK_DISPLAY0("#error Agent :: Jvmti failed to do
popFrame.\n");
Post by Hohensee, Paul
nsk_jvmti_agentFailed();
} else {
- if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(ResumeThread,
jvmti, thread)) ) {
+ if ( ! NSK_JVMTI_VERIFY (jvmti->ResumeThread(thread)) )
{
Post by Hohensee, Paul
=>
+ if (! NSK_JVMTI_VERIFY( jvmti->GetThreadState(thread, &state)) ){


+ if ( ! NSK_JVMTI_VERIFY ( jvmti->PopFrame(thread) ) ) {


+ if ( ! NSK_JVMTI_VERIFY ( jvmti->ResumeThread(thread))
) {
Post by Hohensee, Paul
In hs301t001.cpp
- if (! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities, jvmti, &caps) )) {
+ if (! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {
=>
+ if (! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) )) {
In hs301t002.cpp
- if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities, jvmti, &caps) ) ) {
+ if ( ! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) ) ) {
=>
+ if ( ! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) ) ) {
In hs301t003.cpp
- if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB4(GetClassSignature,
- jvmti_env, klass, &className, &generic) ) ) {
+ if ( ! NSK_JVMTI_VERIFY (jvmti_env->GetClassSignature(klass,
&className, &generic) ) ) {
=>
+ if ( ! NSK_JVMTI_VERIFY ( jvmti_env->GetClassSignature(klass,
&className, &generic) ) ) {
and
- if (! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities, jvmti, &caps) )) {
+ if (! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {
=>
+ if (! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) )) {
and
- if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities,
- jvmti, &caps) )) {
+ if ( ! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {
=>
+ if ( ! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) )) {
*Date: *Tuesday, October 16, 2018 at 4:24 PM
*Subject: *Re: RFR (L) 8211899: Remove the NSK_CPP_STUB macros from
vmTestbase for jvmti/scenarios/[E-M]
Hi all,
How about on a Tuesday? :)
Sneak peak and motivational sentence: after this goes in, we have this
one <http://cr.openjdk.java.net/%7Ejcbeyler/8212148/webrev.00/> which
removes the NSK_CPP_STUBs from the tests entirely! :)
Jc
Hi all,
Any chance for a second reviewer on a Friday? :)
Jc
On Thu, Oct 11, 2018 at 11:03 AM Alex Menkov
got it.
LGTM.
--alex
Post by JC Beyler
Hi Alex,
Thanks for the review! Yes I had seen that too before sending
this
Post by JC Beyler
https://bugs.openjdk.java.net/browse/JDK-8211905
Which now is merged and I've updated my webrev to reflect
this of course.
Post by JC Beyler
My rationale for not doing it here directly is always the
same: keeping
Post by JC Beyler
the changes to the "spirit" of what the change is trying to
do. Those
Post by JC Beyler
extra casts were a bit out-of-scope and so I just forked the
fix in 8211905.
Post by JC Beyler
Thanks!
Jc
On Wed, Oct 10, 2018 at 5:40 PM Alex Menkov
Hi Jc,
Overall looks good.
- jclassName = (jstring) (jstring) (jstring) (jstring)
(jstring)
Post by JC Beyler
(jstring) (jstring) (jstring) (jstring)
NSK_CPP_STUB3(CallObjectMethod,
Post by JC Beyler
jni_env, klass,
- methodID);
+ jclassName = (jstring) (jstring) (jstring) (jstring)
(jstring)
Post by JC Beyler
(jstring) (jstring) (jstring) (jstring)
jni_env->CallObjectMethod(klass,
methodID);
Please drop multi "(jstring)"
--alex
Post by JC Beyler
Hi all,
I am continuing the NSK_CPP_STUB removal with this
next webrev.
http://cr.openjdk.java.net/~jcbeyler/8211899/webrev.00/
<http://cr.openjdk.java.net/%7Ejcbeyler/8211899/webrev.00/>
Post by JC Beyler
<
http://cr.openjdk.java.net/%7Ejcbeyler/8211899/webrev.00/>
Post by Hohensee, Paul
<http://cr.openjdk.java.net/%7Ejcbeyler/8211899/webrev.00/>
Post by JC Beyler
Post by JC Beyler
Bug: https://bugs.openjdk.java.net/browse/JDK-8211899
The change is still straight-forward though, since it
is just
Post by JC Beyler
doing the
Post by JC Beyler
same NSK_CPP_STUB removal. However when I was looking
at the
Post by JC Beyler
changes, a
Post by JC Beyler
lot of these changes are touching lines with spaces
before/after
Post by JC Beyler
Post by JC Beyler
parenthesis. I've almost never touched the spaces
except if I was
Post by JC Beyler
Post by JC Beyler
refactoring by hand the line at the same time. The
rationale
Post by JC Beyler
being that
Post by JC Beyler
the lines will get fixed a few more times and are, at
worse,
Post by JC Beyler
covered by
https://bugs.openjdk.java.net/browse/JDK-8211335, which
Post by JC Beyler
I've
Post by JC Beyler
commited to doing.
Two exceptions are here where I pushed out the code
into
Post by Hohensee, Paul
Post by JC Beyler
assignments due
Post by JC Beyler
- 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
Post by Hohensee, Paul
Post by JC Beyler
Post by JC Beyler
-
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
Post by Hohensee, Paul
Post by JC Beyler
Post by JC Beyler
And one exception here where a commented line was
doing the
Post by JC Beyler
out-of-if
Post by JC Beyler
assignment so I just uncommented it and used the
- 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
Post by Hohensee, Paul
Post by JC Beyler
Post by JC Beyler
I've tested the modified changes on my machine, all
modified
Post by JC Beyler
tests pass.
Post by JC Beyler
Let me know what you think,
Jc
Ps: 2 more of these and we can say good bye to
NSK_CPP_STUB*
Post by JC Beyler
--
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
David Holmes
2018-10-18 02:26:20 UTC
Permalink
Post by JC Beyler
Hi all,
@Serguei: no worries for the time taken, I am just two webrevs away from
conquering the NSK_CPP_STUBs, I might have been a bit pushy for a review
:-) I apologize!
@David: It's not hotspot style but a lot of those files had that style.
if ( before_we_had_this ) // space after ( + space before )
if (now_we_have_this )  // only space before )
which is kind of weird and Paul's comment was to at least try to not
make it inconsistent to that point.
I have one webrev to go that will remove NSK_CPP_STUBs and then I'm
going to fix all the spaces (via
https://bugs.openjdk.java.net/browse/JDK-8211335) for these .cpp files
so we won't have the issue anymore.
Does that make sense?
Yes thanks for clarifying.

David
Post by JC Beyler
Thanks!
Jc
Re spaces :).  There’s a bunch of places where “(jvmti_env”
 should be
“( jvmti_env”, but of course not all of them. I found a bunch.
Why should there be a space after the '(' ? That's not hotspot-style.
Neither is a space before ')'.
David
Otherwise, lgtm., no need for another webrev.
Paul
In hs104t002.cpp
-        if (! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities,
jvmti,
-                &caps) )) {
+        if (! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {
=>
+        if (! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) )) {
In hs203t003.cpp
-    if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB4(GetClassSignature,
-                    jvmti_env, klass, &className, &generic) ) ) {
+    if ( ! NSK_JVMTI_VERIFY (jvmti_env->GetClassSignature(klass,
&className, &generic) ) ) {
=>
+    if ( ! NSK_JVMTI_VERIFY ( jvmti_env->GetClassSignature(klass,
&className, &generic) ) ) {
and
-        if (! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities,
jvmti,
&caps) )) {
+        if (! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {
=>
+        if (! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) )) {
and
-    if ( ! NSK_JVMTI_VERIFY( NSK_CPP_STUB3(GetThreadState, jvmti,
thread, &state) )  ) {
+    if ( ! NSK_JVMTI_VERIFY(jvmti->GetThreadState(thread,
&state) )  ) {
=>
+    if ( ! NSK_JVMTI_VERIFY( jvmti->GetThreadState(thread,
&state) )  ) {
and
-    if ( ! NSK_JVMTI_VERIFY( NSK_CPP_STUB3(GetThreadState, jvmti,
thread, &state) )  ) {
+    if ( ! NSK_JVMTI_VERIFY(jvmti->GetThreadState(thread,
&state) )  ) {
           nsk_printf(" Agent :: Error while getting thread
state.\n");
           nsk_jvmti_agentFailed();
       } else {
           if ( state & JVMTI_THREAD_STATE_SUSPENDED) {
-            if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2( PopFrame,
jvmti,
thread) ) ) {
+            if ( ! NSK_JVMTI_VERIFY (jvmti->PopFrame(thread) ) ) {
=>
+    if ( ! NSK_JVMTI_VERIFY( jvmti->GetThreadState(thread,
&state) )  ) {

+            if ( ! NSK_JVMTI_VERIFY ( jvmti->PopFrame(thread) ) ) {
and
-    if ( !NSK_JVMTI_VERIFY( NSK_CPP_STUB2 ( ResumeThread, jvmti,
thread)) ) {
+    if ( !NSK_JVMTI_VERIFY(jvmti->ResumeThread(thread)) ) {
=>
+    if ( !NSK_JVMTI_VERIFY( jvmti->ResumeThread(thread)) ) {
In hs203t004.cpp
-                if ( ! NSK_JVMTI_VERIFY (
NSK_CPP_STUB2(GenerateEvents,
jvmti_env,
-                                JVMTI_EVENT_COMPILED_METHOD_LOAD
) )) {
+                if ( ! NSK_JVMTI_VERIFY
(jvmti_env->GenerateEvents(JVMTI_EVENT_COMPILED_METHOD_LOAD) )) {
=>
+                if ( ! NSK_JVMTI_VERIFY (
jvmti_env->GenerateEvents(JVMTI_EVENT_COMPILED_METHOD_LOAD) )) {
and
-        if ( ! NSK_JVMTI_VERIFY (
NSK_CPP_STUB3(GetMethodDeclaringClass,
-                        jvmti_env, method, &threadClass) ) ) {
+        if ( ! NSK_JVMTI_VERIFY
(jvmti_env->GetMethodDeclaringClass(method, &threadClass) ) ) {
=>
+        if ( ! NSK_JVMTI_VERIFY (
jvmti_env->GetMethodDeclaringClass(method, &threadClass) ) ) {
and
-        if (! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities,
jvmti,
&caps) )) {
+        if (! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {
=>
+        if (! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) )) {
and
-    if (  NSK_JVMTI_VERIFY( NSK_CPP_STUB2(SuspendThread, jvmti,
thread)
) ) {
+    if (  NSK_JVMTI_VERIFY(jvmti->SuspendThread(thread) ) ) {
=>
+    if (  NSK_JVMTI_VERIFY( jvmti->SuspendThread(thread) ) ) {
and
-    if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB3(GetThreadState, jvmti,
-                    thread, &state) ) ) {
+    if ( ! NSK_JVMTI_VERIFY (jvmti->GetThreadState(thread,
&state) ) ) {
           NSK_COMPLAIN0("#error Agent :: while getting thread's
state.\n");
           nsk_jvmti_agentFailed();
       } else {
           if ( state & JVMTI_THREAD_STATE_SUSPENDED) {
-            if ( ! NSK_JVMTI_VERIFY( NSK_CPP_STUB2(PopFrame, jvmti,
thread) ) ){
+            if ( ! NSK_JVMTI_VERIFY(jvmti->PopFrame(thread) ) ){
=>
+    if ( ! NSK_JVMTI_VERIFY ( jvmti->GetThreadState(thread,
&state) ) ) {

+            if ( ! NSK_JVMTI_VERIFY( jvmti->PopFrame(thread) ) ){
In hs204t001.cpp
-        /* if( (myTestClass =NSK_CPP_STUB2(NewGlobalRef,jni_env,
klass)) == NULL) {
+        /* if( (myTestClass =jni_env->NewGlobalRef(klass)) ==
NULL) {
=>
+        /* if( (myTestClass = jni_env->NewGlobalRef(klass)) ==
NULL) {
and
-    if (!NSK_JNI_VERIFY(env, (testClass =(jclass)
NSK_CPP_STUB2(NewGlobalRef, env, klass)) != NULL))
+    if (!NSK_JNI_VERIFY(env, (testClass =(jclass)
env->NewGlobalRef(klass)) != NULL))
           nsk_jvmti_setFailStatus();
-    if (!NSK_JNI_VERIFY(env, (testedThread
=NSK_CPP_STUB2(NewGlobalRef,
env, thread)) != NULL))
+    if (!NSK_JNI_VERIFY(env, (testedThread
=env->NewGlobalRef(thread))
!= NULL))
=>
+    if (!NSK_JNI_VERIFY(env, (testClass = (jclass)
env->NewGlobalRef(klass)) != NULL))
           nsk_jvmti_setFailStatus();

+    if (!NSK_JNI_VERIFY(env, (testedThread =
env->NewGlobalRef(thread))
!= NULL))
and
-            if (!NSK_JVMTI_VERIFY(  NSK_CPP_STUB2(SuspendThread,
jvmti,
thread))) {
+            if (!NSK_JVMTI_VERIFY(  jvmti->SuspendThread(thread))) {
=>
+            if (!NSK_JVMTI_VERIFY(jvmti->SuspendThread(thread))) {
In hs204t003.cpp
-    if (! NSK_JVMTI_VERIFY( NSK_CPP_STUB3(GetThreadState, jvmti,
thread, &state)) ){
+    if (! NSK_JVMTI_VERIFY(jvmti->GetThreadState(thread, &state)) ){
           NSK_DISPLAY0(" Agent :: Error getting thread state.\n");
           nsk_jvmti_agentFailed();
       } else {
           if ( state & JVMTI_THREAD_STATE_SUSPENDED) {
               NSK_DISPLAY0(" Agent :: Thread state =
JVMTI_THREAD_STATE_SUSPENDED.\n");
-            if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(PopFrame,
jvmti,
thread) ) ) {
+            if ( ! NSK_JVMTI_VERIFY (jvmti->PopFrame(thread) ) ) {
                   NSK_DISPLAY0("#error Agent :: Jvmti failed to
do popFrame.\n");
                   nsk_jvmti_agentFailed();
               } else {
-                if ( ! NSK_JVMTI_VERIFY (
NSK_CPP_STUB2(ResumeThread,
jvmti, thread)) ) {
+                if ( ! NSK_JVMTI_VERIFY
(jvmti->ResumeThread(thread)) ) {
=>
+    if (! NSK_JVMTI_VERIFY( jvmti->GetThreadState(thread,
&state)) ){

+            if ( ! NSK_JVMTI_VERIFY ( jvmti->PopFrame(thread) ) ) {

+                if ( ! NSK_JVMTI_VERIFY (
jvmti->ResumeThread(thread)) ) {
In hs301t001.cpp
-        if (! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities,
jvmti,
&caps) )) {
+        if (! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) )) {
=>
+        if (! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) )) {
In hs301t002.cpp
-        if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities,
jvmti,
&caps) ) ) {
+        if ( ! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) ) ) {
=>
+        if ( ! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps)
) ) {
In hs301t003.cpp
-  if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB4(GetClassSignature,
-                  jvmti_env, klass, &className, &generic) ) ) {
+  if ( ! NSK_JVMTI_VERIFY (jvmti_env->GetClassSignature(klass,
&className, &generic) ) ) {
=>
+  if ( ! NSK_JVMTI_VERIFY ( jvmti_env->GetClassSignature(klass,
&className, &generic) ) ) {
and
-        if (! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities,
jvmti,
&caps) ))  {
+        if (! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) ))  {
=>
+        if (! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps) ))  {
and
-        if ( ! NSK_JVMTI_VERIFY ( NSK_CPP_STUB2(AddCapabilities,
-                        jvmti, &caps) ))  {
+        if ( ! NSK_JVMTI_VERIFY (jvmti->AddCapabilities(&caps) ))  {
=>
+        if ( ! NSK_JVMTI_VERIFY ( jvmti->AddCapabilities(&caps)
))  {
*From: *serviceability-dev
*Date: *Tuesday, October 16, 2018 at 4:24 PM
*Subject: *Re: RFR (L) 8211899: Remove the NSK_CPP_STUB macros from
vmTestbase for jvmti/scenarios/[E-M]
Hi all,
How about on a Tuesday? :)
Sneak peak and motivational sentence: after this goes in, we have
this
one <http://cr.openjdk.java.net/%7Ejcbeyler/8212148/webrev.00/>
which
removes the NSK_CPP_STUBs from the tests entirely! :)
Jc
     Hi all,
     Any chance for a second reviewer on a Friday? :)
     Jc
     On Thu, Oct 11, 2018 at 11:03 AM Alex Menkov
         got it.
         LGTM.
         --alex
          > Hi Alex,
          >
          > Thanks for the review! Yes I had seen that too before
sending
         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
          >
          >     Hi Jc,
          >
          >     Overall looks good.
          >
          >
          >
          >     -    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
          >
          >      > Hi all,
          >      >
          >      > I am continuing the NSK_CPP_STUB removal with this
         next 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/>
          >
 <http://cr.openjdk.java.net/%7Ejcbeyler/8211899/webrev.00/>
          >      >
         <http://cr.openjdk.java.net/%7Ejcbeyler/8211899/webrev.00/>
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
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
          >      > -
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
          >      > -
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
--
Thanks,
Jc
--
s***@oracle.com
2018-10-17 22:45:58 UTC
Permalink
Hi Jc,

++LGTM.

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

Thanks,
Serguei
Post by Alex Menkov
got it.
LGTM.
--alex
Post by JC Beyler
Hi Alex,
Thanks for the review! Yes I had seen that too before sending this
https://bugs.openjdk.java.net/browse/JDK-8211905
Which now is merged and I've updated my webrev to reflect this of course.
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
    Hi Jc,
    Overall looks good.
    -    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
     > 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
     > - 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
     > - 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
Loading...