Discussion:
RFR (M) 212939: Add space after if/while/for/switch and parenthesis
(too old to reply)
JC Beyler
2018-11-07 19:20:32 UTC
Permalink
Hi all,

Could I get a review for a relatively straight-forward space transformation
webrev? This adds spaces between keywords and '(' and fixes the case "){"
across C++ files in vmTestbase.

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

Thanks!
Jc
Chris Plummer
2018-11-07 20:23:30 UTC
Permalink
Hi JC.

In Callbacks.cpp you missed the needed extra indent on the lines
following the "if":

 406       if (!NSK_JVMTI_VERIFY(jvmti->GetFieldName(targetClass,
 407 targetFields[field],
 408 &objects_info[object].fields[field].name,
 409 &objects_info[object].fields[field].signature,
 410 NULL)))

 481       if ((objects_info[object].fields[field].primitive &&
!objects_info[object].collected)
 482          || !objects_info[object].fields[field].collected) {

Otherwise looks good. I don't need to see another webrev.

thanks,

Chris
Post by JC Beyler
Hi all,
Could I get a review for a relatively straight-forward space
transformation webrev? This adds spaces between keywords and '(' and
fixes the case "){" across C++ files in vmTestbase.
Webrev: http://cr.openjdk.java.net/~jcbeyler/8212939/webrev.00/
<http://cr.openjdk.java.net/%7Ejcbeyler/8212939/webrev.00/>
Bug: https://bugs.openjdk.java.net/browse/JDK-8212939
Thanks!
Jc
s***@oracle.com
2018-11-07 21:01:21 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>
+1 LGTM.<br>
<br>
One question:<br>
  Would it be convenient at the same time to fix missing spaces
around commas and comparisons in loops and conditions.<br>
  One more case is function call arguments but I did not see any
instances of it.<br>
<br>
  Some examples:<br>
<br>
 
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Ejcbeyler/8212939/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/Breakpoint/breakpoint001/breakpoint001.cpp.udiff.html">http://cr.openjdk.java.net/%7Ejcbeyler/8212939/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/Breakpoint/breakpoint001/breakpoint001.cpp.udiff.html</a><br>
<pre><span class="removed">- for(i=0; i&lt;METH_NUM; i++)</span>
<span class="new">+ for (i=0; i&lt;METH_NUM; i++)</span></pre>
<br>
 
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Ejcbeyler/8212939/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/ClassLoad/classload001/classload001.cpp.udiff.html">http://cr.openjdk.java.net/%7Ejcbeyler/8212939/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/ClassLoad/classload001/classload001.cpp.udiff.html</a><br>
<pre><span class="removed">- for(i=0; i&lt;EXP_SIG_NUM; i++)</span>
<span class="new">+ for (i=0; i&lt;EXP_SIG_NUM; i++)</span>
clsEvents[i] = 0;

<span class="removed">- for(i=0; i&lt;UNEXP_SIG_NUM; i++)</span>
<span class="new">+ for (i=0; i&lt;UNEXP_SIG_NUM; i++)</span></pre>
<br>
<br>
 
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Ejcbeyler/8212939/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/stress/jni/libjnistress004.cpp.udiff.html">http://cr.openjdk.java.net/%7Ejcbeyler/8212939/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/stress/jni/libjnistress004.cpp.udiff.html</a><br>
<br>
  There are many cases like below:<br>
<pre><span class="removed">- for(i=0;i&lt;DIGESTLENGTH;i++) {</span>
<span class="new">+ for (i=0;i&lt;DIGESTLENGTH;i++) {</span></pre>
. . .<br>
<pre><span class="removed">- for(i=0;i&lt;len;i++)</span>
<span class="removed">- if(ch[i]!=tmp[i]) {</span>
<span class="new">+ for (i=0;i&lt;len;i++)</span>
<span class="new">+ if (ch[i]!=tmp[i]) {</span></pre>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 11/7/18 12:23, Chris Plummer wrote:<br>
</div>
<blockquote type="cite"
cite="mid:d1e9ec86-8432-bcce-aabe-***@oracle.com">Hi JC.
<br>
<br>
In Callbacks.cpp you missed the needed extra indent on the lines
following the "if":
<br>
<br>
 406       if
(!NSK_JVMTI_VERIFY(jvmti-&gt;GetFieldName(targetClass,
<br>
 407 targetFields[field],
<br>
 408 &amp;objects_info[object].fields[field].name,
<br>
 409 &amp;objects_info[object].fields[field].signature,
<br>
 410 NULL)))
<br>
<br>
 481       if ((objects_info[object].fields[field].primitive
&amp;&amp; !objects_info[object].collected)
<br>
 482          || !objects_info[object].fields[field].collected) {
<br>
<br>
Otherwise looks good. I don't need to see another webrev.
<br>
<br>
thanks,
<br>
<br>
Chris
<br>
<br>
On 11/7/18 11:20 AM, JC Beyler wrote:
<br>
<blockquote type="cite">Hi all,
<br>
<br>
Could I get a review for a relatively straight-forward space
transformation webrev? This adds spaces between keywords and '('
and fixes the case "){" across C++ files in vmTestbase.
<br>
<br>
Webrev: <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~jcbeyler/8212939/webrev.00/">http://cr.openjdk.java.net/~jcbeyler/8212939/webrev.00/</a> <a class="moz-txt-link-rfc2396E" href="http://cr.openjdk.java.net/%7Ejcbeyler/8212939/webrev.00/">&lt;http://cr.openjdk.java.net/%7Ejcbeyler/8212939/webrev.00/&gt;</a>
<br>
Bug: <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8212939">https://bugs.openjdk.java.net/browse/JDK-8212939</a>
<br>
<br>
Thanks!
<br>
Jc
<br>
</blockquote>
<br>
<br>
<br>
</blockquote>
<br>
</body>
</html>
JC Beyler
2018-11-07 21:59:51 UTC
Permalink
Hi Serguei,

I have https://bugs.openjdk.java.net/browse/JDK-8212960 open for that. I
like doing things in small contained steps for reviewing and sanity
checking. But if you want, I can run a script to fix all these cases now
and resent a RFR.
I created https://bugs.openjdk.java.net/browse/JDK-8213499 for the
arguments.

Let me know what you prefer, I can see the pros/cons of both ways :-),
Jc
Post by s***@oracle.com
Hi Jc,
+1 LGTM.
Would it be convenient at the same time to fix missing spaces around
commas and comparisons in loops and conditions.
One more case is function call arguments but I did not see any instances
of it.
http://cr.openjdk.java.net/%7Ejcbeyler/8212939/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/Breakpoint/breakpoint001/breakpoint001.cpp.udiff.html
- for(i=0; i<METH_NUM; i++)+ for (i=0; i<METH_NUM; i++)
http://cr.openjdk.java.net/%7Ejcbeyler/8212939/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/ClassLoad/classload001/classload001.cpp.udiff.html
- for(i=0; i<EXP_SIG_NUM; i++)+ for (i=0; i<EXP_SIG_NUM; i++)
clsEvents[i] = 0;
- for(i=0; i<UNEXP_SIG_NUM; i++)+ for (i=0; i<UNEXP_SIG_NUM; i++)
http://cr.openjdk.java.net/%7Ejcbeyler/8212939/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/stress/jni/libjnistress004.cpp.udiff.html
- for(i=0;i<DIGESTLENGTH;i++) {+ for (i=0;i<DIGESTLENGTH;i++) {
. . .
- for(i=0;i<len;i++)- if(ch[i]!=tmp[i]) {+ for (i=0;i<len;i++)+ if (ch[i]!=tmp[i]) {
Thanks,
Serguei
Hi JC.
In Callbacks.cpp you missed the needed extra indent on the lines following
406 if (!NSK_JVMTI_VERIFY(jvmti->GetFieldName(targetClass,
407 targetFields[field],
408 &objects_info[object].fields[field].name,
409 &objects_info[object].fields[field].signature,
410 NULL)))
481 if ((objects_info[object].fields[field].primitive &&
!objects_info[object].collected)
482 || !objects_info[object].fields[field].collected) {
Otherwise looks good. I don't need to see another webrev.
thanks,
Chris
Hi all,
Could I get a review for a relatively straight-forward space
transformation webrev? This adds spaces between keywords and '(' and fixes
the case "){" across C++ files in vmTestbase.
Webrev: http://cr.openjdk.java.net/~jcbeyler/8212939/webrev.00/
<http://cr.openjdk.java.net/%7Ejcbeyler/8212939/webrev.00/>
<http://cr.openjdk.java.net/%7Ejcbeyler/8212939/webrev.00/>
Bug: https://bugs.openjdk.java.net/browse/JDK-8212939
Thanks!
Jc
--
Thanks,
Jc
s***@oracle.com
2018-11-07 22:08:13 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>
It is Okay if it is covered by different bugs.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 11/7/18 13:59, JC Beyler wrote:<br>
</div>
<blockquote type="cite"
cite="mid:***@mail.gmail.com">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">Hi Serguei,<br>
<div><br>
</div>
<div>I have <a
href="https://bugs.openjdk.java.net/browse/JDK-8212960"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8212960</a>
open for that. I like doing things in small contained
steps for reviewing and sanity checking. But if you want,
I can run a script to fix all these cases now and resent a
RFR.</div>
<div>I created <a
href="https://bugs.openjdk.java.net/browse/JDK-8213499"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8213499</a>
for the arguments.</div>
<div><br>
</div>
<div>Let me know what you prefer, I can see the pros/cons of
both ways :-),</div>
<div>Jc</div>
</div>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Wed, Nov 7, 2018 at 1:01 PM <a
href="mailto:***@oracle.com"
moz-do-not-send="true">***@oracle.com</a> &lt;<a
href="mailto:***@oracle.com"
moz-do-not-send="true">***@oracle.com</a>&gt;
wrote:<br>
</div>
<blockquote class="gmail_quote">
<div>
<div class="m_8217570621416410833moz-cite-prefix">Hi Jc,<br>
<br>
+1 LGTM.<br>
<br>
One question:<br>
  Would it be convenient at the same time to fix missing
spaces around commas and comparisons in loops and
conditions.<br>
  One more case is function call arguments but I did not
see any instances of it.<br>
<br>
  Some examples:<br>
<br>
 
<a class="m_8217570621416410833moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejcbeyler/8212939/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/Breakpoint/breakpoint001/breakpoint001.cpp.udiff.html"
target="_blank" moz-do-not-send="true">http://cr.openjdk.java.net/%7Ejcbeyler/8212939/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/Breakpoint/breakpoint001/breakpoint001.cpp.udiff.html</a><br>
<pre><span class="m_8217570621416410833removed">- for(i=0; i&lt;METH_NUM; i++)</span>
<span class="m_8217570621416410833new">+ for (i=0; i&lt;METH_NUM; i++)</span></pre>
<br>
 
<a class="m_8217570621416410833moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejcbeyler/8212939/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/ClassLoad/classload001/classload001.cpp.udiff.html"
target="_blank" moz-do-not-send="true">http://cr.openjdk.java.net/%7Ejcbeyler/8212939/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/ClassLoad/classload001/classload001.cpp.udiff.html</a><br>
<pre><span class="m_8217570621416410833removed">- for(i=0; i&lt;EXP_SIG_NUM; i++)</span>
<span class="m_8217570621416410833new">+ for (i=0; i&lt;EXP_SIG_NUM; i++)</span>
clsEvents[i] = 0;

<span class="m_8217570621416410833removed">- for(i=0; i&lt;UNEXP_SIG_NUM; i++)</span>
<span class="m_8217570621416410833new">+ for (i=0; i&lt;UNEXP_SIG_NUM; i++)</span></pre>
<br>
<br>
 
<a class="m_8217570621416410833moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejcbeyler/8212939/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/stress/jni/libjnistress004.cpp.udiff.html"
target="_blank" moz-do-not-send="true">http://cr.openjdk.java.net/%7Ejcbeyler/8212939/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/stress/jni/libjnistress004.cpp.udiff.html</a><br>
<br>
  There are many cases like below:<br>
<pre><span class="m_8217570621416410833removed">- for(i=0;i&lt;DIGESTLENGTH;i++) {</span>
<span class="m_8217570621416410833new">+ for (i=0;i&lt;DIGESTLENGTH;i++) {</span></pre>
. . .<br>
<pre><span class="m_8217570621416410833removed">- for(i=0;i&lt;len;i++)</span>
<span class="m_8217570621416410833removed">- if(ch[i]!=tmp[i]) {</span>
<span class="m_8217570621416410833new">+ for (i=0;i&lt;len;i++)</span>
<span class="m_8217570621416410833new">+ if (ch[i]!=tmp[i]) {</span></pre>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 11/7/18 12:23, Chris Plummer wrote:<br>
</div>
<blockquote type="cite">Hi JC. <br>
<br>
In Callbacks.cpp you missed the needed extra indent on the
lines following the "if": <br>
<br>
 406       if
(!NSK_JVMTI_VERIFY(jvmti-&gt;GetFieldName(targetClass, <br>
 407 targetFields[field], <br>
 408 &amp;objects_info[object].fields[field].name, <br>
 409 &amp;objects_info[object].fields[field].signature, <br>
 410 NULL))) <br>
<br>
 481       if
((objects_info[object].fields[field].primitive &amp;&amp;
!objects_info[object].collected) <br>
 482          ||
!objects_info[object].fields[field].collected) { <br>
<br>
Otherwise looks good. I don't need to see another webrev.
<br>
<br>
thanks, <br>
<br>
Chris <br>
<br>
On 11/7/18 11:20 AM, JC Beyler wrote: <br>
<blockquote type="cite">Hi all, <br>
<br>
Could I get a review for a relatively straight-forward
space transformation webrev? This adds spaces between
keywords and '(' and fixes the case "){" across C++
files in vmTestbase. <br>
<br>
Webrev: <a
class="m_8217570621416410833moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejcbeyler/8212939/webrev.00/"
target="_blank" moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8212939/webrev.00/</a>
<a class="m_8217570621416410833moz-txt-link-rfc2396E"
href="http://cr.openjdk.java.net/%7Ejcbeyler/8212939/webrev.00/"
target="_blank" moz-do-not-send="true">&lt;http://cr.openjdk.java.net/%7Ejcbeyler/8212939/webrev.00/&gt;</a>
<br>
Bug: <a
class="m_8217570621416410833moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8212939"
target="_blank" moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8212939</a>
<br>
<br>
Thanks! <br>
Jc <br>
</blockquote>
<br>
<br>
<br>
</blockquote>
<br>
</div>
</blockquote>
</div>
<br>
<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>
<br>
</body>
</html>

Loading...