RFR (M) 8211261: Remove the NSK_CPP_STUB macros from vmTestbase for jvmti/[A-G]*

JC Beyler jcbeyler at google.com
Mon Oct 1 16:50:26 UTC 2018

Hi Chris,

Thanks for the review!

I think most of your comments are/should be future items, I inlined my

On Fri, Sep 28, 2018 at 7:49 PM Chris Plummer <chris.plummer at oracle.com>

> 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)) ) {
I saw that too, that will disappear when I remove the NSK_JVMTI_VERIFY
altogether. These spaces were there before so I did not
want to try to fix them in this fix-up round since anyway we were going to
touch these lines again anyway.

I looked and there are a lot of these across the vmTestbase folders.

I created this bug to track it:
(To be honest, every time I look a bit at the lines fixed right now, I see
things around I'm itching to fix next but I strive to keep the
transformation simple).

> 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) {
I already have this on my future plate:

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;
For this, I saw a conversation in hotspot (
saying we have to be careful here. I think returning != 0 will work, right?
If so, I'll create a bug to fix all of these up.

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

Done for the lines and the white space at the end of the line.

Basically, my ordering of refactoring would be if the reviewers agree:
   - Remove the NSK_CPP_STUB*  (which is what these refactoring to do)
   - Remove the NSK_*_VERIFY macros at which point I'll remove that space
you saw for NSK_*_VERIFY lines; that will remove the ) in the lines
   - Move out the assignments
   - Remove the remaining spaces before a ) for l

Let me know what you think,

> 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


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181001/a9a55a0a/attachment.html>

More information about the serviceability-dev mailing list