|
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:
-> 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
-> 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?
Thanks all!
Jc
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
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
--
--
|
