RFR (L) 8214417: Add space after/before {} in vmTestbase/nsk/jvmti/[A-I] tests
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Fri Nov 30 02:45:53 UTC 2018
+1
Thanks,
Serguei
On 11/29/18 4:44 PM, David Holmes wrote:
> +1
>
> This is burning a lot of cycles. :)
>
> David
>
> On 30/11/2018 9:57 am, Alex Menkov wrote:
>> Looks ok
>>
>> --alex
>>
>> On 11/29/2018 15:00, JC Beyler wrote:
>>> 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
>>> <alexey.menkov at oracle.com <mailto:alexey.menkov at oracle.com>> 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, serguei.spitsyn at oracle.com
>>> <mailto: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
>>> <mailto: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
>>> <mailto: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
>>> >>>>>>>
>>> >>>>>
>>> >>>
>>> >
>>>
>>>
>>>
>>> --
>>>
>>> Thanks,
>>> Jc
More information about the serviceability-dev
mailing list