Discussion:
RFR (L) 8212770: Remove spaces before/after () for vmTestbase/jvmti/[s-u]
(too old to reply)
JC Beyler
2018-10-22 18:29:54 UTC
Permalink
Hi all,

Here is the next webrev (second out of 3) to remove the spaces after/before
() from vmTestbase. It is straightforward and I generated the webrev with
white-space changes showing up of course:

Webrev: http://cr.openjdk.java.net/~jcbeyler/8212770/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8212770

Could I please get some reviews?

Thanks,
Jc
Alex Menkov
2018-10-22 20:37:33 UTC
Permalink
LGTM.

--alex
Post by JC Beyler
Hi all,
Here is the next webrev (second out of 3) to remove the spaces
after/before () from vmTestbase. It is straightforward and I generated
Webrev: http://cr.openjdk.java.net/~jcbeyler/8212770/webrev.00/
<http://cr.openjdk.java.net/%7Ejcbeyler/8212770/webrev.00/>
Bug: https://bugs.openjdk.java.net/browse/JDK-8212770
Could I please get some reviews?
Thanks,
Jc
JC Beyler
2018-10-24 20:00:57 UTC
Permalink
Hi all,

Anybody else want to review this? We can close the book on spaces
before/after () once this one goes in :)
Jc
Post by Alex Menkov
LGTM.
--alex
Post by JC Beyler
Hi all,
Here is the next webrev (second out of 3) to remove the spaces
after/before () from vmTestbase. It is straightforward and I generated
Webrev: http://cr.openjdk.java.net/~jcbeyler/8212770/webrev.00/
<http://cr.openjdk.java.net/%7Ejcbeyler/8212770/webrev.00/>
Bug: https://bugs.openjdk.java.net/browse/JDK-8212770
Could I please get some reviews?
Thanks,
Jc
--
Thanks,
Jc
Chris Plummer
2018-10-24 21:57:51 UTC
Permalink
<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>
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-&gt;GetStaticObjectField(debugeeClass,
rootFieldID)) != NULL )) {<br>
+                jni-&gt;GetStaticObjectField(debugeeClass,
rootFieldID)) != NULL)) {<br>
<br>
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>
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 ) &amp;&amp;
(redefineNumber == 0) ) {<br>
+    if ((strcmp(name, CLASS_NAME) ==0) &amp;&amp; (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>
thanks,<br>
<br>
Chris<br>
<br>
On 10/24/18 1:00 PM, JC Beyler wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAF9BGBw8mcFCDXoAUVgomS=iT0x7oQELNTXf-***@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<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 &lt;<a
href="mailto:***@oracle.com"
moz-do-not-send="true">***@oracle.com</a>&gt;
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">LGTM.<br>
<br>
--alex<br>
<br>
On 10/22/2018 11:29, JC Beyler wrote:<br>
&gt; Hi all,<br>
&gt; <br>
&gt; Here is the next webrev (second out of 3) to remove the
spaces <br>
&gt; after/before () from vmTestbase. It is straightforward
and I generated <br>
&gt; the webrev with white-space changes showing up of course:<br>
&gt; <br>
&gt; 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>
&gt; &lt;<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>&gt;<br>
&gt; 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>
&gt; <br>
&gt; Could I please get some reviews?<br>
&gt; <br>
&gt; Thanks,<br>
&gt; Jc<br>
</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>
JC Beyler
2018-10-25 02:54:05 UTC
Permalink
Hi Chris,
Post by Chris Plummer
Hi JC,
I see some cases of changes on lines where there is an assignment in a
conditional. Will these conflict with your other webrev?
if (!NSK_JNI_VERIFY(jni, (root =
- jni->GetStaticObjectField(debugeeClass, rootFieldID)) !=
NULL )) {
+ jni->GetStaticObjectField(debugeeClass, rootFieldID)) !=
NULL)) {
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 :)
Post by Chris Plummer
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
- if (nsk_jvmti_parseOptions(options) == NSK_FALSE ) {
+ if (nsk_jvmti_parseOptions(options) == NSK_FALSE) {
I can't find it so I created:
https://bugs.openjdk.java.net/browse/JDK-8212959
Post by Chris Plummer
The following is missing a space. This piece of code is probably
replicated at least a dozen times.
- if ( rc!= JNI_OK ) {
+ if (rc!= JNI_OK) {
And a missing space here also. Only saw it in one place.
- if ( (strcmp(name, CLASS_NAME) ==0 ) && (redefineNumber == 0) ) {
+ if ((strcmp(name, CLASS_NAME) ==0) && (redefineNumber == 0)) {
- if ( nsk_jvmti_redefineClass(jvmti_env, klass, fileName) ==
NSK_TRUE ) {
+ if (nsk_jvmti_redefineClass(jvmti_env, klass, fileName) ==
NSK_TRUE) {
For all of these, I created:
https://bugs.openjdk.java.net/browse/JDK-8212960

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

thanks,
Post by Chris Plummer
Chris
Hi all,
Anybody else want to review this? We can close the book on spaces
before/after () once this one goes in :)
Jc
Post by Alex Menkov
LGTM.
--alex
Post by JC Beyler
Hi all,
Here is the next webrev (second out of 3) to remove the spaces
after/before () from vmTestbase. It is straightforward and I generated
Webrev: http://cr.openjdk.java.net/~jcbeyler/8212770/webrev.00/
<http://cr.openjdk.java.net/%7Ejcbeyler/8212770/webrev.00/>
Bug: https://bugs.openjdk.java.net/browse/JDK-8212770
Could I please get some reviews?
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
Chris Plummer
2018-10-25 04:32:05 UTC
Permalink
<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 &lt;<a href="mailto:***@oracle.com"
target="_blank" moz-do-not-send="true">***@oracle.com</a>&gt;
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-&gt;GetStaticObjectField(debugeeClass,
rootFieldID)) != NULL )) {<br>
+               
jni-&gt;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 ) &amp;&amp;
(redefineNumber == 0) ) {<br>
+    if ((strcmp(name, CLASS_NAME) ==0) &amp;&amp;
(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 &lt;<a
href="mailto:***@oracle.com"
target="_blank" moz-do-not-send="true">***@oracle.com</a>&gt;
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>
&gt; Hi all,<br>
&gt; <br>
&gt; Here is the next webrev (second out of 3)
to remove the spaces <br>
&gt; after/before () from vmTestbase. It is
straightforward and I generated <br>
&gt; the webrev with white-space changes showing
up of course:<br>
&gt; <br>
&gt; 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>
&gt; &lt;<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>&gt;<br>
&gt; 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>
&gt; <br>
&gt; Could I please get some reviews?<br>
&gt; <br>
&gt; Thanks,<br>
&gt; 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>
JC Beyler
2018-10-25 15:44:01 UTC
Permalink
Thanks Chris,

Pushed then!
Jc
Post by Chris Plummer
Hi JC,
webrev.00 is fine since you have bugs filed for the rest.
thanks,
Chris
Hi Chris,
Post by Chris Plummer
Hi JC,
I see some cases of changes on lines where there is an assignment in a
conditional. Will these conflict with your other webrev?
if (!NSK_JNI_VERIFY(jni, (root =
- jni->GetStaticObjectField(debugeeClass, rootFieldID)) !=
NULL )) {
+ jni->GetStaticObjectField(debugeeClass, rootFieldID)) !=
NULL)) {
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 :)
Post by Chris Plummer
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
- if (nsk_jvmti_parseOptions(options) == NSK_FALSE ) {
+ if (nsk_jvmti_parseOptions(options) == NSK_FALSE) {
https://bugs.openjdk.java.net/browse/JDK-8212959
Post by Chris Plummer
The following is missing a space. This piece of code is probably
replicated at least a dozen times.
- if ( rc!= JNI_OK ) {
+ if (rc!= JNI_OK) {
And a missing space here also. Only saw it in one place.
- if ( (strcmp(name, CLASS_NAME) ==0 ) && (redefineNumber == 0) ) {
+ if ((strcmp(name, CLASS_NAME) ==0) && (redefineNumber == 0)) {
- if ( nsk_jvmti_redefineClass(jvmti_env, klass, fileName) ==
NSK_TRUE ) {
+ if (nsk_jvmti_redefineClass(jvmti_env, klass, fileName) ==
NSK_TRUE) {
https://bugs.openjdk.java.net/browse/JDK-8212960
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,
Jc
thanks,
Post by Chris Plummer
Chris
Hi all,
Anybody else want to review this? We can close the book on spaces
before/after () once this one goes in :)
Jc
Post by Alex Menkov
LGTM.
--alex
Post by JC Beyler
Hi all,
Here is the next webrev (second out of 3) to remove the spaces
after/before () from vmTestbase. It is straightforward and I generated
Webrev: http://cr.openjdk.java.net/~jcbeyler/8212770/webrev.00/
<http://cr.openjdk.java.net/%7Ejcbeyler/8212770/webrev.00/>
Bug: https://bugs.openjdk.java.net/browse/JDK-8212770
Could I please get some reviews?
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
Loading...