<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Thanks!<br>
<br>
On 10/5/18 4:02 PM, JC Beyler wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAF9BGBxqH=8EgiPX23DXxt22L_6HLh3p7tGwf48YafRq2hcceA@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<div dir="ltr">
<div dir="ltr">Just for completeness:Â </div>
<div dir="ltr"><br>
</div>
<div dir="ltr">@Chris, I created <a
href="https://bugs.openjdk.java.net/browse/JDK-8211802"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8211802</a>
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.</div>
<div><br>
</div>
<div>Thanks!</div>
<div>Jc</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Mon, Oct 1, 2018 at 11:59 AM Chris Plummer
<<a href="mailto:chris.plummer@oracle.com"
moz-do-not-send="true">chris.plummer@oracle.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF">
<div class="m_-1319000943628777899moz-cite-prefix">On
10/1/18 9:50 AM, JC Beyler wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">Hi Chris,
<div><br>
</div>
<div>Thanks for the review!</div>
<div><br>
</div>
<div>I think most of your comments are/should be
future items, I inlined my answers:</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Fri, Sep 28, 2018 at 7:49 PM
Chris Plummer <<a
href="mailto:chris.plummer@oracle.com"
target="_blank" moz-do-not-send="true">chris.plummer@oracle.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote"
style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">Hi JC,<br>
<br>
In addition to Alex's ForceEarlyReturn001.cpp
comment:<br>
<br>
There are many places where I see a space
between two parens at the end <br>
of the line. For example, in
attach020Agent00.cpp:<br>
<br>
  168    if
(!NSK_JVMTI_VERIFY(jvmti->AddCapabilities(&caps))
) {<br>
<br>
</blockquote>
<div><br>
</div>
<div>I saw that too, that will disappear when I
remove the NSK_JVMTI_VERIFY altogether. These
spaces were there before so I did not</div>
<div>want to try to fix them in this fix-up
round since anyway we were going to touch
these lines again anyway. </div>
<div><br>
</div>
<div>I looked and there are a lot of these
across the vmTestbase folders.</div>
<div><br>
</div>
<div>I created this bug to track it:Â <a
href="https://bugs.openjdk.java.net/browse/JDK-8211335"
target="_blank" moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8211335</a></div>
<div>(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).</div>
<div>Â </div>
<blockquote class="gmail_quote"
style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex"> Every
place NSK_JNI_VERIFY is used there is ugliness
with "if" <br>
expressions involving JNI results that are not
already boolean. Besides <br>
a boolean being computed for the JNI result,
often there is also an <br>
assignment to the JNI result. I'm not sure if
you have plans to clean <br>
stuff like this up, but it would be best to
always do the assignment <br>
first, even if currently there is no local
variable being assigned to. <br>
It would simplify the boolean expression being
passed to NSK_JNI_VERIFY. <br>
Here's one example:<br>
<br>
  137    if (!NSK_JNI_VERIFY(jni, (array =
(jbyteArray)<br>
  138           Â
jni->GetStaticObjectField(cls, fieldID)) !=
NULL)) {<br>
<br>
This would be much better:<br>
<br>
  137    array =
(jbyteArray)jni->GetStaticObjectField(cls,
fieldID);<br>
  138    if (!NSK_JNI_VERIFY(jni, array !=
NULL) {<br>
<br>
</blockquote>
<div><br>
</div>
<div>I already have this on my future plate:</div>
<div><a
href="https://bugs.openjdk.java.net/browse/JDK-8210922"
target="_blank" moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8210922</a><br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
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.<br>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_quote">
<div><br>
</div>
<blockquote class="gmail_quote"
style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
attach015Target.cpp: use of ?: is not needed
and it should be explicitly <br>
checking if FindClass is NULL.<br>
<br>
  40    return
jni->FindClass(LOADED_CLASS_NAME) ?
JNI_TRUE : JNI_FALSE;<br>
<br>
</blockquote>
<div><br>
</div>
<div>For this, I saw a conversation in hotspot (<a
href="http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-August/033828.html"
target="_blank" moz-do-not-send="true">http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-August/033828.html</a>)
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.</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
Yes, that is what i was suggesting, except compare to NULL,
no 0.<br>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_quote">
<div>Â <br>
</div>
<blockquote class="gmail_quote"
style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
attach022Agent00.cpp: The empty line 88 and
141 should be removed. Also <br>
extra space near the end of the line:<br>
</blockquote>
<div><br>
</div>
<div>Done for the lines and the white space at
the end of the line.</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
ok.<br>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_quote">
<div><br>
</div>
<div><br>
</div>
<div>
<div>Basically, my ordering of refactoring
would be if the reviewers agree:</div>
<div>Â Â - Remove the NSK_CPP_STUB*Â (which is
what these refactoring to do)</div>
<div>Â Â - 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</div>
<div>Â Â - Move out the assignments</div>
  - Remove the remaining spaces before a )
for l</div>
<div>Â </div>
<div>Let me know what you think,</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
Looks good.<br>
<br>
Chris<br>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_quote">
<div>Jc</div>
<div><br>
</div>
<blockquote class="gmail_quote"
style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex"> <br>
thanks,<br>
<br>
Chris<br>
<br>
On 9/27/18 10:15 PM, JC Beyler wrote:<br>
> Hi all,<br>
><br>
> I continued the NSK_CPP_STUB removal with
this webrev:<br>
><br>
> Webrev: <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8211261/webrev.00/"
rel="noreferrer" target="_blank"
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8211261/webrev.00/</a>
<br>
> <<a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8211261/webrev.00/"
rel="noreferrer" target="_blank"
moz-do-not-send="true">http://cr.openjdk.java.net/%7Ejcbeyler/8211261/webrev.00/</a>><br>
> Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8211261"
rel="noreferrer" target="_blank"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8211261</a><br>
><br>
> This does the another set of 50 files of
the jvmti subfolder, it is <br>
> done automatically by the scripts I put
in the bug comments.<br>
><br>
> This passes the tests changed on my dev
machine.<br>
><br>
> Let me know what you think,<br>
> Jc<br>
<br>
<br>
<br>
</blockquote>
</div>
<br clear="all">
<div><br>
</div>
-- <br>
<div dir="ltr"
class="m_-1319000943628777899gmail-m_3622677733063014630m_3563255169388879489gmail-m_-2163559460033312510m_-1086457419127703162gmail-m_-1699793861447603098m_6813899786495674574m_556791172373509929gmail_signature">
<div dir="ltr">
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<p><br>
</p>
</div>
</blockquote>
</div>
<br clear="all">
<div><br>
</div>
-- <br>
<div dir="ltr" class="gmail_signature"
data-smartmail="gmail_signature">
<div dir="ltr">
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</blockquote>
<p><br>
</p>
</body>
</html>