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

Wed, 12 Sep 2018 14:10:48 -0700

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:

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 <[email protected]> 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.

This passes testing for the changed tests on my dev machine.

Thanks for your reviews,
Jc



--

Thanks,
Jc


Reply via email to