[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR (L) 8212770: Remove spaces before/after () for vmTestbase/jvmti/[s-u]
From: JC Beyler <jcbeyler () google ! com>
Date: 2018-10-25 15:44:01
Message-ID: CAF9BGBwAXhMDwfc2Hm2gJ+36yHqc7acnX0SZ_xekUodo5UJ-GA () mail ! gmail ! com
[Download RAW message or body]
Thanks Chris,
Pushed then!
Jc
On Wed, Oct 24, 2018 at 9:32 PM Chris Plummer <chris.plummer@oracle.com>
wrote:
> Hi JC,
>
> webrev.00 is fine since you have bugs filed for the rest.
>
> thanks,
>
> Chris
>
> On 10/24/18 7:54 PM, JC Beyler wrote:
>
> Hi Chris,
>
> I inlined my answers:
>
> On Wed, Oct 24, 2018 at 2:58 PM Chris Plummer <chris.plummer@oracle.com>
> wrote:
>
>> Hi JC,
>>
>> Overall looks pretty good:
>>
>> 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 :)
>
>
>> 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:
>>
>> - 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
>
>
>
>> 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)) {
>>
>> There's a double space here:
>>
>> - 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,
>>
>> Chris
>>
>> On 10/24/18 1:00 PM, JC Beyler wrote:
>>
>> Hi all,
>>
>> Anybody else want to review this? We can close the book on spaces
>> before/after () once this one goes in :)
>> Jc
>>
>> On Mon, Oct 22, 2018 at 1:37 PM Alex Menkov <alexey.menkov@oracle.com>
>> wrote:
>>
>>> LGTM.
>>>
>>> --alex
>>>
>>> On 10/22/2018 11:29, JC Beyler wrote:
>>> > 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/
>>> > <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
[Attachment #3 (text/html)]
<div dir="ltr">Thanks Chris,<div><br></div><div>Pushed \
then!</div><div>Jc</div></div><br><div class="gmail_quote"><div dir="ltr">On Wed, Oct \
24, 2018 at 9:32 PM Chris Plummer <<a \
href="mailto:chris.plummer@oracle.com">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_-1504811353305104248moz-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">
<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:chris.plummer@oracle.com" \
target="_blank">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"> <div \
bgcolor="#FFFFFF">
<div \
class="m_-1504811353305104248gmail-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="m_-1504811353305104248gmail-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">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="m_-1504811353305104248gmail-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" \
target="_blank">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="m_-1504811353305104248gmail-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:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@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">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">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">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="m_-1504811353305104248gmail-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="m_-1504811353305104248gmail-m_-5441643586961841865gmail_signature"> <div \
dir="ltr"> <div><br>
</div>
Thanks,
<div>Jc</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>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic