<div dir="ltr">Thanks all, and pushed :)<div>Jc</div></div><br><div class="gmail_quote"><div dir="ltr">On Thu, Nov 29, 2018 at 6:45 PM <<a href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">+1<br>
<br>
Thanks,<br>
Serguei<br>
<br>
On 11/29/18 4:44 PM, David Holmes wrote:<br>
> +1<br>
><br>
> This is burning a lot of cycles. :)<br>
><br>
> David<br>
><br>
> On 30/11/2018 9:57 am, Alex Menkov wrote:<br>
>> Looks ok<br>
>><br>
>> --alex<br>
>><br>
>> On 11/29/2018 15:00, JC Beyler wrote:<br>
>>> Hi Alex,<br>
>>><br>
>>> Yes that is true, I had initially only done the one-liner {} because <br>
>>> I was unsure about the cases above :-)<br>
>>><br>
>>> Here is a webrev handling all cases for those 40 files:<br>
>>> Webrev: <a href="http://cr.openjdk.java.net/~jcbeyler/8214417/webrev.01/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~jcbeyler/8214417/webrev.01/</a><br>
>>> Bug: <a href="https://bugs.openjdk.java.net/browse/JDK-8214417" rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8214417</a><br>
>>><br>
>>> This was tested on my dev machine for all changed tests.<br>
>>><br>
>>> Thanks!<br>
>>> Jc<br>
>>><br>
>>> On Thu, Nov 29, 2018 at 2:26 PM Alex Menkov <br>
>>> <<a href="mailto:alexey.menkov@oracle.com" target="_blank">alexey.menkov@oracle.com</a> <mailto:<a href="mailto:alexey.menkov@oracle.com" target="_blank">alexey.menkov@oracle.com</a>>> wrote:<br>
>>><br>
>>> Â Â Â Ok, guys, you win :)<br>
>>><br>
>>> Â Â Â About the fix:<br>
>>> Â Â Â Looks like only 1-line initializers are fixed, so now we have mixed<br>
>>> Â Â Â style in some files like:<br>
>>><br>
>>> Â Â Â ClearFieldAccessWatch/clrfldw001/clrfldw001.cpp (lines 52-53 are <br>
>>> not<br>
>>> Â Â Â updated):<br>
>>><br>
>>> Â Â Â NULL },<br>
>>> Â Â Â 51Â Â Â { "nsk/jvmti/ClearFieldAccessWatch/clrfldw001", "fld1", <br>
>>> "I", 1,<br>
>>> Â Â Â NULL },<br>
>>> Â Â Â 52Â Â Â {"nsk/jvmti/ClearFieldAccessWatch/clrfldw001", "fld2",<br>
>>> Â Â Â 53 Â "Lnsk/jvmti/ClearFieldAccessWatch/clrfldw001a;", 0, NULL},<br>
>>> Â Â Â 54Â Â Â { "nsk/jvmti/ClearFieldAccessWatch/clrfldw001a", "fld3",<br>
>>> Â Â Â "[I", 0,<br>
>>> Â Â Â NULL },<br>
>>><br>
>>><br>
>>> Â Â Â The same in ClearFieldModificationWatch/clrfmodw001/clrfmodw001.cpp<br>
>>> Â Â Â (lines 52-53):<br>
>>> Â Â Â 51Â Â Â { "nsk/jvmti/ClearFieldModificationWatch/clrfmodw001", <br>
>>> "fld1",<br>
>>> Â Â Â "I", 1, NULL },<br>
>>> Â Â Â 52 Â {"nsk/jvmti/ClearFieldModificationWatch/clrfmodw001", "fld2",<br>
>>> Â Â Â 53 Â "Lnsk/jvmti/ClearFieldModificationWatch/clrfmodw001a;",<br>
>>> Â Â Â 0, NULL},<br>
>>> Â Â Â 54Â Â Â { "nsk/jvmti/ClearFieldModificationWatch/clrfmodw001a", <br>
>>> "fld3",<br>
>>> Â Â Â "[I", 0, NULL },<br>
>>><br>
>>><br>
>>> Â Â Â GetClassSignature/getclsig006/getclsig006.cpp (44-51)<br>
>>> Â Â Â 43Â Â Â { "getclsig006", <br>
>>> "Lnsk/jvmti/GetClassSignature/getclsig006;",<br>
>>> Â Â Â "NULL" },<br>
>>> Â Â Â 44Â Â Â {"getclsig006b", <br>
>>> "Lnsk/jvmti/GetClassSignature/getclsig006b;",<br>
>>> Â Â Â 45 Â "<L:Ljava/lang/String;>Ljava/lang/Object;"},<br>
>>> Â Â Â 46Â Â Â {"getclsig006c", <br>
>>> "Lnsk/jvmti/GetClassSignature/getclsig006c;",<br>
>>> Â Â Â 47 <br>
>>> Â "<A:Ljava/lang/Object;B:Ljava/lang/Integer;>Ljava/lang/Object;"},<br>
>>><br>
>>> Â Â Â (and others files)<br>
>>><br>
>>> Â Â Â --alex<br>
>>><br>
>>> Â Â Â On 11/29/2018 11:32, Daniel D. Daugherty wrote:<br>
>>> Â Â Â Â > I would also write code like what David shows... :-)<br>
>>> Â Â Â Â ><br>
>>> Â Â Â Â > Dan<br>
>>> Â Â Â Â ><br>
>>> Â Â Â Â ><br>
>>> Â Â Â Â > On 11/29/18 5:48 AM, David Holmes wrote:<br>
>>> Â Â Â Â >> I vote for the spaces around { and }<br>
>>> Â Â Â Â >><br>
>>> Â Â Â Â >> I would write:<br>
>>> Â Â Â Â >><br>
>>> Â Â Â Â >> int foo() { return 1; }<br>
>>> Â Â Â Â >><br>
>>> Â Â Â Â >> so would also write:<br>
>>> Â Â Â Â >><br>
>>> Â Â Â Â >> static jvmtiEvent testEvents[] = { JVMTI_EVENT_THREAD_START,<br>
>>> Â Â Â Â >> JVMTI_EVENT_THREAD_END };<br>
>>> Â Â Â Â >><br>
>>> Â Â Â Â >> Cheers,<br>
>>> Â Â Â Â >> David<br>
>>> Â Â Â Â >><br>
>>> Â Â Â Â >> On 29/11/2018 8:29 pm, <a href="mailto:serguei.spitsyn@oracle.com" target="_blank">serguei.spitsyn@oracle.com</a><br>
>>> Â Â Â <mailto:<a href="mailto:serguei.spitsyn@oracle.com" target="_blank">serguei.spitsyn@oracle.com</a>> wrote:<br>
>>> Â Â Â Â >>> Hi Alex,<br>
>>> Â Â Â Â >>><br>
>>> Â Â Â Â >>><br>
>>> Â Â Â Â >>> On 11/28/18 18:18, Alex Menkov wrote:<br>
>>> Â Â Â Â >>>> Hi Serguei,<br>
>>> Â Â Â Â >>>><br>
>>> Â Â Â Â >>>> On 11/28/2018 16:44, <a href="mailto:serguei.spitsyn@oracle.com" target="_blank">serguei.spitsyn@oracle.com</a><br>
>>> Â Â Â <mailto:<a href="mailto:serguei.spitsyn@oracle.com" target="_blank">serguei.spitsyn@oracle.com</a>> wrote:<br>
>>> Â Â Â Â >>>>> There are some implicit rules, like unification and <br>
>>> consistency.<br>
>>> Â Â Â Â >>>>> We want a space or new line after '{' and before '}'.<br>
>>> Â Â Â Â >>>><br>
>>> Â Â Â Â >>>> I believe this rule is about braces for code blocks (new line<br>
>>> Â Â Â after<br>
>>> Â Â Â Â >>>> "{", "}" on a separate line), but this are array <br>
>>> initializers.<br>
>>> Â Â Â Â >>><br>
>>> Â Â Â Â >>> Array initializers are blocs as well.<br>
>>> Â Â Â Â >>> It make it a base for unification.<br>
>>> Â Â Â Â >>><br>
>>> Â Â Â Â >>><br>
>>> Â Â Â Â >>>> And to me for 1-line array initializers spaces do not improve<br>
>>> Â Â Â Â >>>> readability.<br>
>>> Â Â Â Â >>><br>
>>> Â Â Â Â >>> I politely disagree, at least, they improve it for me. :)<br>
>>> Â Â Â Â >>><br>
>>> Â Â Â Â >>> We can do one simple test.<br>
>>> Â Â Â Â >>> What suggestion would make the Jc's awk script simpler?<br>
>>> Â Â Â Â >>> If yours then I'm out.<br>
>>> Â Â Â Â >>> Otherwise, why does it make simpler for script but not for <br>
>>> humans?<br>
>>> Â Â Â Â >>><br>
>>> Â Â Â Â >>> Also, we can wait for one more opinion.<br>
>>> Â Â Â Â >>><br>
>>> Â Â Â Â >>><br>
>>> Â Â Â Â >>>> So to me this looks like the last item from "Whitespace" is<br>
>>> Â Â Â Â >>>> applicable here:<br>
>>> Â Â Â Â >>>> <cite><br>
>>> Â Â Â Â >>>> Try not to change whitespace unless it improves <br>
>>> readability or<br>
>>> Â Â Â Â >>>> consistency. (Different editors indent differently, and <br>
>>> spurious<br>
>>> Â Â Â Â >>>> indentation changes will just make integrations more <br>
>>> difficult.)<br>
>>> Â Â Â Â >>>> </cite><br>
>>> Â Â Â Â >>><br>
>>> Â Â Â Â >>> It was Okay to break this rule when we decided to fix <br>
>>> spacing over<br>
>>> Â Â Â Â >>> all the tests.<br>
>>> Â Â Â Â >>><br>
>>> Â Â Â Â >>> Thanks,<br>
>>> Â Â Â Â >>> Serguei<br>
>>> Â Â Â Â >>><br>
>>> Â Â Â Â >>><br>
>>> Â Â Â Â >>>> --alex<br>
>>> Â Â Â Â >>>><br>
>>> Â Â Â Â >>>>> Why this case needs to have an exception?<br>
>>> Â Â Â Â >>>>><br>
>>> Â Â Â Â >>>>> Thanks,<br>
>>> Â Â Â Â >>>>> Serguei<br>
>>> Â Â Â Â >>>>><br>
>>> Â Â Â Â >>>>><br>
>>> Â Â Â Â >>>>> On 11/28/18 16:18, Alex Menkov wrote:<br>
>>> Â Â Â Â >>>>>> I don't see such rule (I suppose<br>
>>> Â Â Â Â >>>>>> <a href="https://wiki.openjdk.java.net/display/HotSpot/StyleGuide" rel="noreferrer" target="_blank">https://wiki.openjdk.java.net/display/HotSpot/StyleGuide</a> is<br>
>>> Â Â Â Â >>>>>> correct link?)<br>
>>> Â Â Â Â >>>>>><br>
>>> Â Â Â Â >>>>>> --alex<br>
>>> Â Â Â Â >>>>>><br>
>>> Â Â Â Â >>>>>> On 11/28/2018 16:05, <a href="mailto:serguei.spitsyn@oracle.com" target="_blank">serguei.spitsyn@oracle.com</a><br>
>>> Â Â Â <mailto:<a href="mailto:serguei.spitsyn@oracle.com" target="_blank">serguei.spitsyn@oracle.com</a>> wrote:<br>
>>> Â Â Â Â >>>>>>> On 11/28/18 15:43, Alex Menkov wrote:<br>
>>> Â Â Â Â >>>>>>>> Hi Jc,<br>
>>> Â Â Â Â >>>>>>>><br>
>>> Â Â Â Â >>>>>>>> In the JDK-8212771 review thread cleanup for "{}" was<br>
>>> Â Â Â requested<br>
>>> Â Â Â Â >>>>>>>> for statements like<br>
>>> Â Â Â Â >>>>>>>><br>
>>> test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/MethodBind/JvmtiTest/JvmtiTest.cpp, <br>
>>><br>
>>><br>
>>> Â Â Â Â >>>>>>>><br>
>>> Â Â Â Â >>>>>>>><br>
>>> Â Â Â Â >>>>>>>> +#define JVMTI_ERROR_CHECK(str,res) if (res !=<br>
>>> Â Â Â JVMTI_ERROR_NONE)<br>
>>> Â Â Â Â >>>>>>>> { printf(str); printf("%d\n",res); return res;}<br>
>>> Â Â Â Â >>>>>>>><br>
>>> Â Â Â Â >>>>>>>> +#define JVMTI_ERROR_CHECK_EXPECTED_ERROR(str,res,err) if<br>
>>> Â Â Â (res<br>
>>> Â Â Â Â >>>>>>>> != err) { printf(str); printf("unexpected error <br>
>>> %d\n",res);<br>
>>> Â Â Â Â >>>>>>>> return res;}<br>
>>> Â Â Â Â >>>>>>>><br>
>>> Â Â Â Â >>>>>>>> I.e. something like ";}" -> "; }"<br>
>>> Â Â Â Â >>>>>>>><br>
>>> Â Â Â Â >>>>>>>><br>
>>> Â Â Â Â >>>>>>>> I don't think changes like<br>
>>> Â Â Â Â >>>>>>>><br>
>>> Â Â Â Â >>>>>>>> -static jvmtiEvent testEvents[] = <br>
>>> {JVMTI_EVENT_THREAD_START,<br>
>>> Â Â Â Â >>>>>>>> JVMTI_EVENT_THREAD_END};<br>
>>> Â Â Â Â >>>>>>>> +static jvmtiEvent testEvents[] = { <br>
>>> JVMTI_EVENT_THREAD_START,<br>
>>> Â Â Â Â >>>>>>>> JVMTI_EVENT_THREAD_END };<br>
>>> Â Â Â Â >>>>>>>><br>
>>> Â Â Â Â >>>>>>>> are required.<br>
>>> Â Â Â Â >>>>>>><br>
>>> Â Â Â Â >>>>>>> It is better to have it - rules are rules.<br>
>>> Â Â Â Â >>>>>>><br>
>>> Â Â Â Â >>>>>>> Thanks,<br>
>>> Â Â Â Â >>>>>>> Serguei<br>
>>> Â Â Â Â >>>>>>><br>
>>> Â Â Â Â >>>>>>>><br>
>>> Â Â Â Â >>>>>>>> --alex<br>
>>> Â Â Â Â >>>>>>>><br>
>>> Â Â Â Â >>>>>>>><br>
>>> Â Â Â Â >>>>>>>> On 11/28/2018 11:20, JC Beyler wrote:<br>
>>> Â Â Â Â >>>>>>>>> Hi all,<br>
>>> Â Â Â Â >>>>>>>>><br>
>>> Â Â Â Â >>>>>>>>> When working on a previous clean-up (JDK-8212771), I was<br>
>>> Â Â Â asked<br>
>>> Â Â Â Â >>>>>>>>> to clean-up also spaces around {}.<br>
>>> Â Â Â Â >>>>>>>>><br>
>>> Â Â Â Â >>>>>>>>> Here is the first batch out of 2 to fix these cases. <br>
>>> Let me<br>
>>> Â Â Â Â >>>>>>>>> know what you think.<br>
>>> Â Â Â Â >>>>>>>>><br>
>>> Â Â Â Â >>>>>>>>> Webrev:<br>
>>> Â Â Â <a href="http://cr.openjdk.java.net/~jcbeyler/8214417/webrev.00/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~jcbeyler/8214417/webrev.00/</a><br>
>>> Â Â Â Â >>>>>>>>> Bug: <a href="https://bugs.openjdk.java.net/browse/JDK-8214417" rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8214417</a><br>
>>> Â Â Â Â >>>>>>>>><br>
>>> Â Â Â Â >>>>>>>>> Thanks for your reviews :-),<br>
>>> Â Â Â Â >>>>>>>>> Jc<br>
>>> Â Â Â Â >>>>>>><br>
>>> Â Â Â Â >>>>><br>
>>> Â Â Â Â >>><br>
>>> Â Â Â Â ><br>
>>><br>
>>><br>
>>><br>
>>> -- <br>
>>><br>
>>> Thanks,<br>
>>> Jc<br>
<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>