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