RFR (M) 8210689: Remove the multi-line old C style for string literals

Alex Menkov alexey.menkov at oracle.com
Sat Sep 22 00:22:09 UTC 2018


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


More information about the serviceability-dev mailing list