<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Hi JC,<br>
<br>
webrev.00 is fine since you have bugs filed for the rest.<br>
<br>
thanks,<br>
<br>
Chris<br>
<br>
On 10/24/18 7:54 PM, JC Beyler wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAF9BGByRHzSuqQ-7mT=yRiUXg7f7H4y42CLoDd1v4V=***@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">Hi Chris,
<div><br>
</div>
<div>I inlined my answers:</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Wed, Oct 24, 2018 at 2:58 PM Chris
Plummer <<a href="mailto:***@oracle.com"
target="_blank" moz-do-not-send="true">***@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">
<div bgcolor="#FFFFFF">
<div
class="gmail-m_-5441643586961841865gmail-m_7022224358586271106moz-cite-prefix">Hi
JC,<br>
<br>
Overall looks pretty good:<br>
<br>
I see some cases of changes on lines where there is
an assignment in a conditional. Will these conflict
with your other webrev?<br>
<br>
if (!NSK_JNI_VERIFY(jni, (root =<br>
-
jni->GetStaticObjectField(debugeeClass,
rootFieldID)) != NULL )) {<br>
+
jni->GetStaticObjectField(debugeeClass,
rootFieldID)) != NULL)) {<br>
<br>
</div>
</div>
</blockquote>
<div><br>
</div>
<div>Yes they might, I was going to go with the first that
goes through and send an incremental for the other one.
To be honest, I kind of was expecting perhaps some more
conversation about extracting the assignments that I'd
have time to fix the conflicts without anyone really
noticing :)</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">
<div bgcolor="#FFFFFF">
<div
class="gmail-m_-5441643586961841865gmail-m_7022224358586271106moz-cite-prefix">
I noticed a number of cases of checking if a boolean
result is true or false. I brought this up once
before. I forgot if you filed a separate CR for it:<br>
<br>
- if (nsk_jvmti_parseOptions(options) ==
NSK_FALSE ) {<br>
+ if (nsk_jvmti_parseOptions(options) ==
NSK_FALSE) {<br>
<br>
</div>
</div>
</blockquote>
<div><br>
</div>
<div>I can't find it so I created: <a
href="https://bugs.openjdk.java.net/browse/JDK-8212959"
target="_blank" moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8212959</a></div>
<div><br>
</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">
<div bgcolor="#FFFFFF">
<div
class="gmail-m_-5441643586961841865gmail-m_7022224358586271106moz-cite-prefix">
The following is missing a space. This piece of code
is probably replicated at least a dozen times.<br>
<br>
- if ( rc!= JNI_OK ) {<br>
+ if (rc!= JNI_OK) {<br>
<br>
And a missing space here also. Only saw it in one
place.<br>
<br>
- if ( (strcmp(name, CLASS_NAME) ==0 ) &&
(redefineNumber == 0) ) {<br>
+ if ((strcmp(name, CLASS_NAME) ==0) &&
(redefineNumber == 0)) {<br>
<br>
There's a double space here:<br>
<br>
- if ( nsk_jvmti_redefineClass(jvmti_env,
klass, fileName) == NSK_TRUE ) {<br>
+ if (nsk_jvmti_redefineClass(jvmti_env,
klass, fileName) == NSK_TRUE) {<br>
<br>
</div>
</div>
</blockquote>
<div><br>
</div>
<div>For all of these, I created:</div>
<div><a
href="https://bugs.openjdk.java.net/browse/JDK-8212960"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8212960</a></div>
<div><br>
</div>
<div>Let me know if these answer your issues and if
therefore I can push webrev.00 in your opinion. Then
I'll work on the potential conflicts for the other
webrev,</div>
<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">
<div bgcolor="#FFFFFF">
<div
class="gmail-m_-5441643586961841865gmail-m_7022224358586271106moz-cite-prefix">
thanks,<br>
<br>
Chris<br>
<br>
On 10/24/18 1:00 PM, JC Beyler wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">Hi all,
<div><br>
</div>
<div>Anybody else want to review this? We can
close the book on spaces before/after () once
this one goes in :)</div>
<div>Jc</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Mon, Oct 22, 2018 at 1:37 PM
Alex Menkov <<a
href="mailto:***@oracle.com"
target="_blank" moz-do-not-send="true">***@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">LGTM.<br>
<br>
--alex<br>
<br>
On 10/22/2018 11:29, JC Beyler wrote:<br>
> Hi all,<br>
> <br>
> Here is the next webrev (second out of 3)
to remove the spaces <br>
> after/before () from vmTestbase. It is
straightforward and I generated <br>
> the webrev with white-space changes showing
up of course:<br>
> <br>
> Webrev: <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8212770/webrev.00/"
rel="noreferrer" target="_blank"
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8212770/webrev.00/</a>
<br>
> <<a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8212770/webrev.00/"
rel="noreferrer" target="_blank"
moz-do-not-send="true">http://cr.openjdk.java.net/%7Ejcbeyler/8212770/webrev.00/</a>><br>
> Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8212770"
rel="noreferrer" target="_blank"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8212770</a><br>
> <br>
> Could I please get some reviews?<br>
> <br>
> Thanks,<br>
> Jc<br>
</blockquote>
</div>
<br clear="all">
<div><br>
</div>
-- <br>
<div dir="ltr"
class="gmail-m_-5441643586961841865gmail-m_7022224358586271106gmail_signature">
<div dir="ltr">
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</blockquote>
<p><br>
</p>
</div>
</blockquote>
</div>
<br clear="all">
<div><br>
</div>
-- <br>
<div dir="ltr"
class="gmail-m_-5441643586961841865gmail_signature">
<div dir="ltr">
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<p><br>
</p>
</body>
</html>