|
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:
Let me know if that works for you,
Jc
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
--
--
--
|
