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

Alex Menkov alexey.menkov at oracle.com
Fri Sep 21 22:44:15 UTC 2018


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/>
> 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>> 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/>
>>     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>> 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>>
>>>         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>> 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/>
>>>>                 Bug: https://bugs.openjdk.java.net/browse/JDK-8210689
>>>>
>>>>                 Let me know what you think,
>>>>                 Jc
>>>>
>>>>
>>>>
>>>>             -- 
>>>>
>>>>             Thanks,
>>>>             Jc
>>>
>>>
>>>
>>>
>>>         -- 
>>>
>>>         Thanks,
>>>         Jc
>>
>>
>>
>>
>>     -- 
>>
>>     Thanks,
>>     Jc
> 
> 
> 
> 
> -- 
> 
> Thanks,
> Jc


More information about the serviceability-dev mailing list