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