[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR (M) 8211261: Remove the NSK_CPP_STUB macros from vmTestbase for jvmti/[A-G]*
From:       Chris Plummer <chris.plummer () oracle ! com>
Date:       2018-09-29 2:49:35
Message-ID: 2521fc07-1ce2-dd35-1e6e-a5fbfe786888 () oracle ! com
[Download RAW message or body]

Hi JC,

In addition to Alex's ForceEarlyReturn001.cpp comment:

There are many places where I see a space between two parens at the end 
of the line. For example, in attach020Agent00.cpp:

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

Every place NSK_JNI_VERIFY is used there is ugliness with "if" 
expressions involving JNI results that are not already boolean. Besides 
a boolean being computed for the JNI result, often there is also an 
assignment to the JNI result. I'm not sure if you have plans to clean 
stuff like this up, but it would be best to always do the assignment 
first, even if currently there is no local variable being assigned to. 
It would simplify the boolean expression being passed to NSK_JNI_VERIFY. 
Here's one example:

   137         if (!NSK_JNI_VERIFY(jni, (array = (jbyteArray)
   138                         jni->GetStaticObjectField(cls, fieldID)) != NULL)) {

This would be much better:

   137         array = (jbyteArray)jni->GetStaticObjectField(cls, fieldID);
   138         if (!NSK_JNI_VERIFY(jni, array != NULL) {

attach015Target.cpp: use of ?: is not needed and it should be explicitly 
checking if FindClass is NULL.

    40         return jni->FindClass(LOADED_CLASS_NAME) ? JNI_TRUE : JNI_FALSE;

attach022Agent00.cpp: The empty line 88 and 141 should be removed. Also 
extra space near the end of the line:

thanks,

Chris

On 9/27/18 10:15 PM, JC Beyler wrote:
> Hi all,
>
> I continued the NSK_CPP_STUB removal with this webrev:
>
> Webrev: http://cr.openjdk.java.net/~jcbeyler/8211261/webrev.00/ 
> <http://cr.openjdk.java.net/%7Ejcbeyler/8211261/webrev.00/>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8211261
>
> This does the another set of 50 files of the jvmti subfolder, it is 
> done automatically by the scripts I put in the bug comments.
>
> This passes the tests changed on my dev machine.
>
> Let me know what you think,
> Jc



[prev in list] [next in list] [prev in thread] [next in thread]