RFR (L) 8214417: Add space after/before {} in vmTestbase/nsk/jvmti/[A-I] tests
JC Beyler
jcbeyler at google.com
Fri Nov 30 02:59:18 UTC 2018
Thanks all, and pushed :)
Jc
On Thu, Nov 29, 2018 at 6:45 PM <serguei.spitsyn at oracle.com> wrote:
> +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
>
>
--
Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181129/90fa65df/attachment-0001.html>
More information about the serviceability-dev
mailing list