RFR (L) 8214417: Add space after/before {} in vmTestbase/nsk/jvmti/[A-I] tests
David Holmes
david.holmes at oracle.com
Fri Nov 30 00:44:44 UTC 2018
+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