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

[JC Beyler]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180912/a58a9dc3/attachment.html>