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

Mon, 08 Oct 2018 15:32:48 -0700

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:


Thanks for the review!
Jc

On Mon, Oct 8, 2018 at 12:46 AM [email protected] <[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:

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