RFR (L) 8214417: Add space after/before {} in vmTestbase/nsk/jvmti/[A-I] tests

Alex Menkov alexey.menkov at oracle.com
Thu Nov 29 22:26:34 UTC 2018


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, serguei.spitsyn at oracle.com wrote:
>>> Hi Alex,
>>>
>>>
>>> On 11/28/18 18:18, Alex Menkov wrote:
>>>> Hi Serguei,
>>>>
>>>> On 11/28/2018 16:44, serguei.spitsyn at oracle.com 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, serguei.spitsyn at oracle.com 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
>>>>>>>
>>>>>
>>>
> 


More information about the serviceability-dev mailing list