|
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
|
