|
Thanks!
On 10/5/18 4:02 PM, JC Beyler wrote:
Just for completeness:
@Chris, I created https://bugs.openjdk.java.net/browse/JDK-8211802
for the first case you were talking about, which is slightly
different than assignments and perhaps we will do them
together or not, we can see when I finish the macro
replacements.
Thanks!
Jc
On
10/1/18 9:50 AM, JC Beyler wrote:
Hi Chris,
Thanks for the review!
I think most of your comments are/should be
future items, I inlined my answers:
Hi JC,
In addition to Alex's ForceEarlyReturn001.cpp
comment:
There are many places where I see a space
between two parens at the end
of the line. For example, in
attach020Agent00.cpp:
168 if
(!NSK_JVMTI_VERIFY(jvmti->AddCapabilities(&caps))
) {
I saw that too, that will disappear when I
remove the NSK_JVMTI_VERIFY altogether. These
spaces were there before so I did not
want to try to fix them in this fix-up
round since anyway we were going to touch
these lines again anyway.
I looked and there are a lot of these
across the vmTestbase folders.
(To be honest, every time I look a bit at
the lines fixed right now, I see things around
I'm itching to fix next but I strive to keep
the transformation simple).
Every
place NSK_JNI_VERIFY is used there is ugliness
with "if"
expressions involving JNI results that are not
already boolean. Besides
a boolean being computed for the JNI result,
often there is also an
assignment to the JNI result. I'm not sure if
you have plans to clean
stuff like this up, but it would be best to
always do the assignment
first, even if currently there is no local
variable being assigned to.
It would simplify the boolean _expression_ being
passed to NSK_JNI_VERIFY.
Here's one example:
137 if (!NSK_JNI_VERIFY(jni, (array =
(jbyteArray)
138
jni->GetStaticObjectField(cls, fieldID)) !=
NULL)) {
This would be much better:
137 array =
(jbyteArray)jni->GetStaticObjectField(cls,
fieldID);
138 if (!NSK_JNI_VERIFY(jni, array !=
NULL) {
I already have this on my future plate:
Ok, but sometimes it's just a comparison of the result, but
no assignment. You end up with the method call and args
starting on one line, and the last args and closing paren on
the next, followed by a compare op. Would be nice to instead
create a local to assign the result to.
attach015Target.cpp: use of ?: is not needed
and it should be explicitly
checking if FindClass is NULL.
40 return
jni->FindClass(LOADED_CLASS_NAME) ?
JNI_TRUE : JNI_FALSE;
Yes, that is what i was suggesting, except compare to NULL,
no 0.
attach022Agent00.cpp: The empty line 88 and
141 should be removed. Also
extra space near the end of the line:
Done for the lines and the white space at
the end of the line.
ok.
Basically, my ordering of refactoring
would be if the reviewers agree:
- Remove the NSK_CPP_STUB* (which is
what these refactoring to do)
- Remove the NSK_*_VERIFY macros at
which point I'll remove that space you saw
for NSK_*_VERIFY lines; that will remove the
) in the lines
- Move out the assignments
- Remove the remaining spaces before a )
for l
Let me know what you think,
Looks good.
Chris
--
|
