RFR (L) 8214417: Add space after/before {} in vmTestbase/nsk/jvmti/[A-I] tests
Alex Menkov
alexey.menkov at oracle.com
Thu Nov 29 23:57:19 UTC 2018
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