Hi Chris,
Thanks for the review!
I think most of your comments are/should be future items, I inlined my
answers:
On Fri, Sep 28, 2018 at 7:49 PM Chris Plummer <chris.plum...@oracle.com>
wrote:
> 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.
I created this bug to track it:
https://bugs.openjdk.java.net/browse/JDK-8211335
(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:
https://bugs.openjdk.java.net/browse/JDK-8210922
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;
>
>
For this, I saw a conversation in hotspot (
http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-August/033828.html)
saying we have to be careful here. I think returning != 0 will work, right?
If so, I'll create a bug to fix all of these up.
> 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.
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,
Jc
> thanks,
>
> Chris
>
> On 9/27/18 10:15 PM, JC Beyler wrote:
> > Hi all,
> >
> > I continued the NSK_CPP_STUB removal with this webrev:
> >
> > Webrev: http://cr.openjdk.java.net/~jcbeyler/8211261/webrev.00/
> > <http://cr.openjdk.java.net/%7Ejcbeyler/8211261/webrev.00/>
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8211261
> >
> > This does the another set of 50 files of the jvmti subfolder, it is
> > done automatically by the scripts I put in the bug comments.
> >
> > This passes the tests changed on my dev machine.
> >
> > Let me know what you think,
> > Jc
>
>
>
>
--
Thanks,
Jc
