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

Thu Sep 13 01:28:10 UTC 2018

I was going to make the changes of the incremental in the same change. Then
I'll work on the other fixes we talked about in subsequent webrevs when I'm
done with the macro refactoring.

ie: this is my fix for the file we were talking about:
http://cr.openjdk.java.net/~jcbeyler/8210665/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/jni_interception/JI05/ji05t001/ji05t001.cpp.udiff.html

Let me know if that works for you,
Jc

On Wed, Sep 12, 2018 at 5:28 PM Chris Plummer <chris.plummer at oracle.com>
wrote:

> It looks fine, but are you going to make the incremental changes part of
> 8210665, or file a different CR for them and fix all the files under it?
>
> thanks,
>
> Chris
>
> On 9/12/18 4:30 PM, JC Beyler wrote:
>
> Yes agreed, I actually have the same preferences for indentation and
> agreed for the multi-line string literals. And for changing the ifs. I
> actually created:
>
> - Assignments in ifs: https://bugs.openjdk.java.net/browse/JDK-8210687
>     -> I've hand-waved the number with a series of greps to about 600
> cases in vmTestbase,  so except if someone has strong beliefs that they
> should not get cleaned up, we should be able to do in one-two webrevs
>     -> For reference I hand-waved to 3k in src/hotspot
>
> - Multi-line string litterals:
> https://bugs.openjdk.java.net/browse/JDK-8210689
>    -> I've hand-waved the number to being 77 cases in vmTestbase, so that
> is do-able in one webrev
>   -> For reference I count 6ish cases in src/hotspot
>
> Does anyone see any issues trying to handle these after the macro cleanup?
>
> For this webrev, I then offer this one, what do you think Chris?
>
> Webrev: http://cr.openjdk.java.net/~jcbeyler/8210665/webrev.02/
> Incremental: http://cr.openjdk.java.net/~jcbeyler/8210665/webrev.00_02/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8210665
>
> Thanks all!
> Jc
>
>
> On Wed, Sep 12, 2018 at 2:09 PM Chris Plummer <chris.plummer at oracle.com>
> wrote:
>
>> Hi JC,
>>
>> Overall I think your proposed ji05t001.cpp changes are a good idea, but
>> I would say "assignments in an if" is the one I care about the most (but
>> others seem to disagree on this). If you think you will get around to
>> fixing "assignments in an if" soon, then I'm fine leaving out my suggestion
>> for cleaning up the one multi-line case. Otherwise I'd like to see it fixed
>> with this CR.
>>
>> Here's another issue you missed that I've fixed in the past:
>>
>>  118     NSK_DISPLAY1("%s JVMTI env: doRedirect: the JNI function table
>> obtained successfully\n\
>>  119 \toverwriting the function GetVersion() ...\n",
>>
>> Maybe it was written before C supported concatenation of string literals.
>>
>> Also you need some consistency with the following:
>>
>>  175     NSK_DISPLAY1("\nagent %s initializer: obtaining the JVMTI env
>> ...\n",
>>  176                  (indx == 0) ? "A" : "B");
>>
>>  185     NSK_DISPLAY1("\nagent %s initializer: the JVMTI env
>> obtained\n\tsetting event callbacks ...\n",
>>  186         (indx == 0)? "A": "B");
>>
>> I prefer the first version, indenting the arguments on the second line to
>> lineup with the first argument on the first line. I only do the latter if I
>> don't include any argument on the first line, which some times makes sense
>> if the method name is long, and possibly also is invoked using an object
>> with a long name (or is the result of a method call itself).
>>
>> thanks,
>>
>> Chris
>>
>> On 9/12/18 1:54 PM, JC Beyler wrote:
>>
>> Hi Chris,
>>
>> Thanks for the review! That remark is exactly what I was thinking for a
>> lot of these files but then this process would go even slower. A lot of the
>> coding style in these tests seem out of sync with the rest of hotspot and
>> what I would normally expect to see. I have forced myself not to touch them
>> yet and, right now, I was working at this in doing similar changes across
>> the board but keeping it simple to review.
>>
>> I (for fun?) went through ji05t001.cpp and looked quickly what I would
>> change (regardless of coding guidelines). Note the quickly, I really only
>> looked at general formatting and what we could/should perhaps do
>> automatically across the others files at some point.
>>
>> I basically noted:
>>   - no {} around the body of an if, regardless if it is a one-liner
>> afterwards
>>   - missing spaces before/after ==, !=, (etc), ?, :
>>   - assignments in an if
>>   - potentially putting back on one line when it seems still readable
>>
>> Would you have a second to look at the incremental change:
>>
>> http://cr.openjdk.java.net/~jcbeyler/8210665/webrev.00_01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/jni_interception/JI05/ji05t001/ji05t001.cpp.udiff.html
>>
>> I'm suggesting however, for this change, to do this:
>>   - only apply your remark on the make the
>>
>>  174     NSK_DISPLAY1("\nagent %s initializer: obtaining the JVMTI env
>> ...\n", (indx==0)?"A":"B");
>>
>> into two lines.
>>
>> And not do the other changes in this incremental to keep vmTestbase
>> relatively consistent across the board for now.
>>
>> Then, when you (and others) tell me what you think are the important
>> items of the list above and what changes were done in the incremental that
>> you think are not useful, I'll file a bug and, when done with other
>> refactoring, start hitting those (we will just need to find reviewers who
>> are willing to go through the more intense/involved webrevs).
>>
>> What do you think and thanks for your all reviews!
>> Jc
>>
>> On Wed, Sep 12, 2018 at 12:10 PM Chris Plummer <chris.plummer at oracle.com>
>> wrote:
>>
>>> Hi JC,
>>>
>>> Just a couple of minor suggestions in ji05t001.cpp:
>>>
>>>  174     NSK_DISPLAY1("\nagent %s initializer: obtaining the JVMTI env
>>> ...\n", (indx==0)?"A":"B");
>>>
>>> I think I like this better as two lines.
>>>
>>>  204     if ((err = jvmti[indx]->SetEventNotificationMode(JVMTI_ENABLE,
>>>  205             JVMTI_EVENT_VM_INIT, NULL)) != JVMTI_ERROR_NONE) { /*
>>> enable event globally */
>>>
>>> If I had my way you'd be making a couple dozen changes to remove
>>> assignments from conditional expressions, but I've been shot down when
>>> suggesting this in the past because doing this is considered within the
>>> coding guidelines. However, when the conditional takes up two lines, it
>>> really does start to become too hard to read. I'd suggest assigning "err"
>>> before the "if".
>>>
>>> thanks,
>>>
>>> Chris
>>>
>>> On 9/12/18 11:45 AM, 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.
>>>
>>> Webrev: http://cr.openjdk.java.net/~jcbeyler/8210665/webrev.00/
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8210665
>>>
>>> This passes testing for the changed tests on my dev machine.
>>>
>>> Thanks for your reviews,
>>> Jc
>>>
>>>
>>>
>>
>> --
>>
>> Thanks,
>> Jc
>>
>>
>>
>
> --
>
> Thanks,
> Jc
>
>
>

-- 

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