Re: RFR (M) 8210665: Clean up JNI_ENV_ARG and factorize the macros for vmTestbase/jvmti[R-U] tests

Wed, 12 Sep 2018 16:44:17 -0700

Hi Jc,

It looks good.
Some minor comments:

http://cr.openjdk.java.net/%7Ejcbeyler/8210665/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetFieldAccessWatch/setfldw005/setfldw005.cpp.udiff.html

+            fields[i].fid = env-> GetStaticFieldID(
+                cls, fields[i].name, fields[i].sig);
. . .
+            fields[i].fid = env->GetFieldID(
+                cls, fields[i].name, fields[i].sig);

  It is better to make the above one-liners.


http://cr.openjdk.java.net/%7Ejcbeyler/8210665/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetFieldAccessWatch/setfldw006/setfldw006.cpp.udiff.html
+            watches[i].fid = env->GetStaticFieldID(
+                cls, watches[i].f_name, watches[i].f_sig);
. . .
+            watches[i].fid = env->GetFieldID(
+                cls, watches[i].f_name, watches[i].f_sig);
  It is better to make the above one-liners.


http://cr.openjdk.java.net/%7Ejcbeyler/8210665/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetFieldModificationWatch/setfmodw006/setfmodw006.cpp.udiff.html

+            watches[i].fid = env->GetStaticFieldID(
+                cls, watches[i].f_name, watches[i].f_sig);
. . .
+            watches[i].fid = env->GetFieldID(
+                cls, watches[i].f_name, watches[i].f_sig);
  It is better to make the above one-liners.

http://cr.openjdk.java.net/%7Ejcbeyler/8210665/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetLocalVariable/setlocal002/setlocal002.cpp.udiff.html
+    mid = env->GetStaticMethodID(
+        cls, "run", "([Ljava/lang/String;Ljava/io/PrintStream;)I");
  It is better to make the above one-liner.


No need in new webrev if you fix the above.

Thanks,
Serguei


On 9/12/18 11:45, JC Beyler wrote:
Hi all,

I am continuing the clean up of the testbase with the next batch, I know this is getting repetitive but bear with me please, after this webrev, we have in vmTestbase:

- 29 more files that have the JNI_ENV* or JVMTI_ENV* macros (for a subsequent webrev)
- 400+ files that have trivial #ifdef __cplusplus macros around the extern "C" and the final } (for a second subsequent webrev)

After those two webrev, we can go to doing more important refactoring to get the vmTestbase in shape to start migrating out of there hopefully.

So, without further ado, here is another one with 50 file changes and 1283 line changes.


This passes testing for the changed tests on my dev machine.

Thanks for your reviews,
Jc

Reply via email to