RFR (M) 8210689: Remove the multi-line old C style for string literals
JC Beyler
jcbeyler at google.com
Mon Sep 24 16:16:15 UTC 2018
Thanks Alex!
Could I get a second review/LGTM ?
Thanks for your help!
Jc
On Fri, Sep 21, 2018 at 5:22 PM Alex Menkov <alexey.menkov at oracle.com>
wrote:
> LGTM.
>
> --alex
>
> On 09/21/2018 17:06, JC Beyler wrote:
> > Hi Alex,
> >
> > Good catch, it was not done on purpose but now fixed:
> >
> > Webrev: http://cr.openjdk.java.net/~jcbeyler/8210689/webrev.03/
> > <http://cr.openjdk.java.net/%7Ejcbeyler/8210689/webrev.03/>
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8210689
> >
> > Let me know if this works for you and thanks for the review,
> > Jc
> >
> > On Fri, Sep 21, 2018 at 3:44 PM Alex Menkov <alexey.menkov at oracle.com
> > <mailto:alexey.menkov at oracle.com>> wrote:
> >
> > Hi Jc,
> >
> > overall looks good (no changes in the logging)
> > except
> >
> test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t003/hs201t003.cpp
> >
> > :
> > - if ((strcmp(name, expMeth) == 0) &&
> > - (strcmp(sig, expSig) == 0)) {
> > - NSK_DISPLAY4("===== %s event received for the tested
> method:\n\
> > -\tID=0x%p name=\"%s\" signature=\"%s\"\n",
> > + if ((strcmp(name, expMeth) == 0) && (strcmp(sig, expSig) == 0))
> {
> > + NSK_DISPLAY4(
> > + "%s event received for the tested method:\n"
> > + "\tID=0x%p name=\"%s\" signature=\"%s\"\n",
> >
> > "===== " is dropped from the beginning of the line
> > I don't know if this is important.
> >
> > --alex
> >
> >
> > On 09/21/2018 14:29, JC Beyler wrote:
> > > Hi Chris,
> > >
> > > Done here:
> > > Webrev: http://cr.openjdk.java.net/~jcbeyler/8210689/webrev.02/
> > <http://cr.openjdk.java.net/%7Ejcbeyler/8210689/webrev.02/>
> > > <http://cr.openjdk.java.net/%7Ejcbeyler/8210689/webrev.02/>
> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8210689
> > >
> > > Anything else? and anybody else motivated to look?
> > >
> > > Thanks again!
> > > Jc
> > >
> > > On Fri, Sep 21, 2018 at 2:07 PM Chris Plummer
> > <chris.plummer at oracle.com <mailto:chris.plummer at oracle.com>
> > > <mailto:chris.plummer at oracle.com
> > <mailto:chris.plummer at oracle.com>>> wrote:
> > >
> > > Hi JC,
> > >
> > > Overall looks good. Just a couple minor edits needed:
> > >
> > > In nativemethbind003.cpp:
> > >
> > > 158 NSK_DISPLAY1("Inside the registerNative()\nFinding
> > class
> > > \"%s\" ...\n", CLASS_SIG);
> > >
> > > This was two lines and you made it one with a \n in the
> middle of
> > > the string.
> > >
> > > In ap12t001.cpp:
> > >
> > > 69 NSK_COMPLAIN2(
> > > 70 "Received unexpected number of ObjectFree
> > events:
> > > %d\n"
> > > 71 "\texpected number: %d",
> > > 72 obj_free, EXP_OBJ_FREE);
> > >
> > > There's no \n at the end of this output (and there never was).
> > > Normally NSK_COMPLAIN is always used with a terminating \n.
> > >
> > > thanks,
> > >
> > > Chris
> > >
> > >
> > > On 9/21/18 1:05 PM, JC Beyler wrote:
> > >> Hi Chris,
> > >>
> > >> Sounds good to me; here it is:
> > >> Webrev:
> > http://cr.openjdk.java.net/~jcbeyler/8210689/webrev.01/
> > <http://cr.openjdk.java.net/%7Ejcbeyler/8210689/webrev.01/>
> > >> <http://cr.openjdk.java.net/%7Ejcbeyler/8210689/webrev.01/>
> > >> Bug: https://bugs.openjdk.java.net/browse/JDK-8210689
> > >>
> > >> I admit I strived to stay consistent and always started a
> > new line
> > >> for the multi-line argument even if the string was not too
> long;
> > >> it's a question of style I believe but it felt more readable
> to
> > >> me. I'll happily change whatever anyone prefers.
> > >>
> > >> This has passed the vmTestbase tests I changed but due to the
> > >> shared changes, I've launched a full vmTestbase testing now.
> > >>
> > >> Let me know what you think,
> > >> Jc
> > >>
> > >> On Fri, Sep 21, 2018 at 10:59 AM Chris Plummer
> > >> <chris.plummer at oracle.com <mailto:chris.plummer at oracle.com>
> > <mailto:chris.plummer at oracle.com <mailto:chris.plummer at oracle.com>>>
> > wrote:
> > >>
> > >> On 9/21/18 10:55 AM, JC Beyler wrote:
> > >>> Hi Chris,
> > >>>
> > >>> I hesitated to be honest and then thought that
> > debug_str was
> > >>> better as you would clearly see that it is a multi-lilne
> > >>> string and what parameters are what. But I'll take your
> > >>> preference (it's relatively the same for me). Quick
> > question
> > >>> though:
> > >>>
> > >>> Do you have a preference between:
> > >>> NSK_COMPLAIN6(
> > >>> "TEST FAILED: %s field \"%s\" has\n"
> > >>> "\tsignature: \"%s\"\n"
> > >>> "\tgeneric signature: \"%s\"\n\n"
> > >>> "\tExpected: \"%s\"\n"
> > >>> "\t\t\"%s\"\n\n",
> > >>> (instance==0)?"instance":"static",
> > >>> fld_sig[idx][0],
> > >>> sign, (gen_sign==NULL)?"NULL":gen_sign,
> > >>> fld_sig[idx][2], fld_sig[idx][3]);
> > >>> or:
> > >>> NSK_COMPLAIN6(
> > >>> "TEST FAILED: %s field \"%s\"
> > has\n\tsignature: \"%s\"\n"
> > >>> "\tgeneric signature:
> > \"%s\"\n\n\tExpected: \"%s\"\n\t\t\"%s\"\n\n",
> > >>> (instance==0)?"instance":"static",
> > >>> fld_sig[idx][0],
> > >>> sign, (gen_sign==NULL)?"NULL":gen_sign,
> > >>> fld_sig[idx][2], fld_sig[idx][3]);
> > >>> I think I like the first because you can clearly see
> > what we want to be printed out; but for code vertical
> > >>> compression, the second is better. What do you think?
> > >> I also prefer the first one.
> > >>
> > >> thanks,
> > >>
> > >> Chris
> > >>> Thanks!
> > >>> Jc
> > >>>
> > >>> On Fri, Sep 21, 2018 at 10:16 AM Chris Plummer
> > >>> <chris.plummer at oracle.com
> > <mailto:chris.plummer at oracle.com> <mailto:chris.plummer at oracle.com
> > <mailto:chris.plummer at oracle.com>>>
> > >>> wrote:
> > >>>
> > >>> Hi JC,
> > >>>
> > >>> I didn't realize you intended to move all the
> strings
> > >>> into a "const char*" first. Seems unnecessary, and I
> > >>> think not as easy to read:
> > >>>
> > >>> 138 const char* debug_str =
> > >>> 139 "TEST FAILED:
> JVMTI_EVENT_CLASS_LOAD
> > >>> event received for\n"
> > >>> 140 "\t a primitive class/array of
> > primitive
> > >>> types with the signature \"%s\"\n";
> > >>> 141 NSK_COMPLAIN1(debug_str, sig);
> > >>>
> > >>> vs
> > >>>
> > >>> 138 NSK_COMPLAIN1("TEST FAILED:
> > >>> JVMTI_EVENT_CLASS_LOAD event received for\n"
> > >>> 139 "\t a primitive
> > class/array of
> > >>> primitive types with the signature \"%s\"\n",
> > >>> 140 sig);
> > >>>
> > >>> or
> > >>>
> > >>> 138 NSK_COMPLAIN1(
> > >>> 139 "TEST FAILED:
> JVMTI_EVENT_CLASS_LOAD
> > >>> event received for\n"
> > >>> 140 "\t a primitive class/array of
> > primitive
> > >>> types with the signature \"%s\"\n",
> > >>> 141 sig);
> > >>>
> > >>> thanks,
> > >>>
> > >>> Chris
> > >>>
> > >>> On 9/21/18 8:00 AM, JC Beyler wrote:
> > >>>> Hi all,
> > >>>>
> > >>>> Is anyone motivated on a Friday to review this ? :)
> > >>>>
> > >>>> It should be fairly straightforward :-)
> > >>>>
> > >>>> Thanks,
> > >>>> Jc
> > >>>>
> > >>>> On Tue, Sep 18, 2018 at 12:07 PM JC Beyler
> > >>>> <jcbeyler at google.com <mailto:jcbeyler at google.com>
> > <mailto:jcbeyler at google.com <mailto:jcbeyler at google.com>>> wrote:
> > >>>>
> > >>>> Hi all,
> > >>>>
> > >>>> Could I have a review for this webrev:
> > >>>>
> > >>>> Webrev:
> > >>>> http://cr.openjdk.java.net/~jcbeyler/8210689/webrev.00/
> > <http://cr.openjdk.java.net/%7Ejcbeyler/8210689/webrev.00/>
> > >>>>
> > <http://cr.openjdk.java.net/%7Ejcbeyler/8210689/webrev.00/>
> > >>>> Bug:
> > https://bugs.openjdk.java.net/browse/JDK-8210689
> > >>>>
> > >>>> Let me know what you think,
> > >>>> Jc
> > >>>>
> > >>>>
> > >>>>
> > >>>> --
> > >>>>
> > >>>> Thanks,
> > >>>> Jc
> > >>>
> > >>>
> > >>>
> > >>>
> > >>> --
> > >>>
> > >>> Thanks,
> > >>> Jc
> > >>
> > >>
> > >>
> > >>
> > >> --
> > >>
> > >> Thanks,
> > >> Jc
> > >
> > >
> > >
> > >
> > > --
> > >
> > > Thanks,
> > > Jc
> >
> >
> >
> > --
> >
> > Thanks,
> > Jc
>
--
Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180924/4ec641d2/attachment-0001.html>
More information about the serviceability-dev
mailing list