Thanks!
Jc
On Wed, Oct 17, 2018 at 6:34 PM David Holmes <david.hol...@oracle.com
<mailto:david.hol...@oracle.com>> wrote:
On 18/10/2018 6:01 AM, Hohensee, Paul wrote:
> 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
<serviceability-dev-boun...@openjdk.java.net
<mailto:serviceability-dev-boun...@openjdk.java.net>>
> on behalf of JC Beyler <jcbey...@google.com
<mailto:jcbey...@google.com>>
> *Date: *Tuesday, October 16, 2018 at 4:24 PM
> *To: *"alexey.men...@oracle.com
<mailto:alexey.men...@oracle.com>" <alexey.men...@oracle.com
<mailto:alexey.men...@oracle.com>>
> *Cc: *"serviceability-dev@openjdk.java.net
<mailto:serviceability-dev@openjdk.java.net>"
> <serviceability-dev@openjdk.java.net
<mailto:serviceability-dev@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/%7Ejcbeyler/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 <jcbey...@google.com
<mailto:jcbey...@google.com>
> <mailto:jcbey...@google.com <mailto:jcbey...@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
> <alexey.men...@oracle.com <mailto:alexey.men...@oracle.com>
<mailto:alexey.men...@oracle.com <mailto:alexey.men...@oracle.com>>>
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> <mailto:alexey.men...@oracle.com
<mailto:alexey.men...@oracle.com>>
> > <mailto:alexey.men...@oracle.com
<mailto:alexey.men...@oracle.com>
> <mailto: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/>
> >
<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
>
>
> --
>
> Thanks,
>
> Jc
>
>
> --
>
> Thanks,
>
> Jc
>
--
Thanks,
Jc
