|
On 9/13/18 8:39 AM, JC Beyler wrote:
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?
Yes. Please push webrev.00 as-is.
thank,s
Chris
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
--
--
--
--
|
