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

[JC Beyler]

Just for completeness:

@Chris, I created https://bugs.openjdk.java.net/browse/JDK-8211802 for the
first case you were talking about, which is slightly different than
assignments and perhaps we will do them together or not, we can see when I
finish the macro replacements.

Thanks!
Jc

On Mon, Oct 1, 2018 at 11:59 AM Chris Plummer <chris.plummer at oracle.com>
wrote:

> On 10/1/18 9:50 AM, JC Beyler wrote:
>
> 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
>
> Ok, but sometimes it's just a comparison of the result, but no assignment.
> You end up with the method call and args starting on one line, and the last
> args and closing paren on the next, followed by a compare op. Would be nice
> to instead create a local to assign the result to.
>
>
> 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.
>
> Yes, that is what i was suggesting, except compare to NULL, no 0.
>
>
>
>> 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.
>
> ok.
>
>
>
> 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,
>
>
> Looks good.
>
> Chris
>
> 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
>
>
>

-- 

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