Re: RFR (XL) 8211801: Remove the NSK_CPP_STUB macros from vmTestbase for jvmti/scenarios/[A-E]

Wed, 10 Oct 2018 10:23:06 -0700 

+1

--alex

On 10/08/2018 15:31, [email protected] wrote:

Hi Jc,

Thank you for the update!
It looks good to me.

Thanks,
Serguei


On 10/8/18 14:53, JC Beyler wrote:

Hi Serguei,
Normally, I try not to fix issues that already exist before the change. For the space after the NULL for example, that would get fixed by https://bugs.openjdk.java.net/browse/JDK-8211335. In this case, these are the only cases where this happens with a NULL so I fixed them.
For the space missing before jvmti->X, I fixed my script to handle this correctly and add them automatically. 

Here is the new webrev:
Webrev: http://cr.openjdk.java.net/~jcbeyler/8211801/webrev.01 <http://cr.openjdk.java.net/%7Ejcbeyler/8211801/webrev.01> 
Bug: https://bugs.openjdk.java.net/browse/JDK-8211801

Thanks for the review!
Jc
On Mon, Oct 8, 2018 at 12:46 AM [email protected] <mailto:[email protected]> <[email protected] <mailto:[email protected]>> wrote: 

    Hi Jc,


    
http://cr.openjdk.java.net/%7Ejcbeyler/8211801/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/allocation/AP06/ap06t001/ap06t001.cpp.udiff.html

              if (!NSK_JNI_VERIFY(jni, (fid =
    - NSK_CPP_STUB4(GetStaticFieldID, jni,
    - debugeeClass,
    - "thread",
    - THREAD_CLS_SIGNATURE)) != NULL )) {
    + jni->GetStaticFieldID(debugeeClass, "thread",
    THREAD_CLS_SIGNATURE)) != NULL )) {
                  nsk_jvmti_setFailStatus();
                  break;
              }
if (!NSK_JNI_VERIFY(jni, (localRefThread = 
    - NSK_CPP_STUB3(GetStaticObjectField, jni,
    - debugeeClass,
    - fid )) != NULL )) {
    + jni->GetStaticObjectField(debugeeClass, fid)) != NULL )) {
                  NSK_COMPLAIN0("GetStaticObjectField returned NULL for 'thread' 
field value\n\n");
                  nsk_jvmti_setFailStatus();
                  break;
              }

       Could you, please, remove extra space after NULL.


    In many-many files (especially, in the foleder: scenarios/capability):

    + if
    
(!NSK_JVMTI_VERIFY_CODE(JVMTI_ERROR_MUST_POSSESS_CAPABILITY,jvmti->SuspendThread(thread)))
    ... + if
    
(!NSK_JVMTI_VERIFY_CODE(JVMTI_ERROR_MUST_POSSESS_CAPABILITY,jvmti->ResumeThread(thread)))
    ... + if
    
(!NSK_JVMTI_VERIFY_CODE(JVMTI_ERROR_MUST_POSSESS_CAPABILITY,jvmti->InterruptThread(thread)))


    There are a lot of cases like above where a space before jvmti->
    is missed.


    Otherwise, looks good.


    Thanks!
    Serguei


    On 10/5/18 16:50, JC Beyler wrote:

    Hi all,

    I continued the NSK_CPP_STUB removal with this next webrev.

    My apologies, this one is big for 50 files; I can further cut it
    up if we prefer. It is straight-forward though, since it is just
    doing the same NSK_CPP_STUB removal.

    Without further ado:
    Webrev: http://cr.openjdk.java.net/~jcbeyler/8211801/webrev.00/
    <http://cr.openjdk.java.net/%7Ejcbeyler/8211801/webrev.00/>
    Bug: https://bugs.openjdk.java.net/browse/JDK-8211801

    This does another 50 file batch. I did the same for
    https://bugs.openjdk.java.net/browse/JDK-8211782 so the scripts
    can be found there (and added a comment in this bug to link to that).

    I've tested the modified changes on my machine, all modified
    tests pass.

    Let me know what you think,
    Jc




--

Thanks,
Jc

Reply via email to