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

[JC Beyler]

Hi Chris,

Thanks for the review!

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

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

> 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:
https://bugs.openjdk.java.net/browse/JDK-8211335
(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:
https://bugs.openjdk.java.net/browse/JDK-8210922

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 (
http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-August/033828.html)
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,
Jc


> 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
>
>
>
>

-- 

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