RFR (M) 8210665: Clean up JNI_ENV_ARG and factorize the macros for vmTestbase/jvmti[R-U] tests
JC Beyler
jcbeyler at google.com
Thu Sep 13 15:39:35 UTC 2018
My intent is to finish these macro cleanups (1 more webrev for the
JNI_ENV/JVMTI_ENV in vmTestbase); then I'd like to do a webrev to remove
the remaining #ifdef cplusplus from vmTestbase (another webrev).
Right after that (so most likely start at some point next week), I'll start
working on these two new bugs we just filed. I have to figure out how to do
it semi-automatically, awk here I come again :).
I got a +1 from Serguei with minor comments, with my current intent of
timing Chris, do I have your LGTM for webrev.00 as is and we postpone the
two nits you saw?
Jc
On Wed, Sep 12, 2018 at 11:16 PM Chris Plummer <chris.plummer at oracle.com>
wrote:
> I'm still not sure of your intent for when you are fixing the various
> issues. I think you should commit your webrev.00 and not do all these
> additional changes as part of that.
>
> Chris
>
> On 9/12/18 6:28 PM, JC Beyler wrote:
>
> 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
>
>
>
--
Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180913/6938f04f/attachment-0001.html>
More information about the serviceability-dev
mailing list