RFR (M) 8210689: Remove the multi-line old C style for string literals
JC Beyler
jcbeyler at google.com
Mon Sep 24 21:30:10 UTC 2018
Thanks Chris!
Because you saw that, I double checked and actually saw a few more so I'm
just showing the incremental here for your information:
http://cr.openjdk.java.net/~jcbeyler/8210689/webrev.03_04/
(The full is here: http://cr.openjdk.java.net/~jcbeyler/8210689/webrev.04/)
I've restarted a manual test and will submit if it passes.
Thanks for the review!
Jc
On Mon, Sep 24, 2018 at 10:52 AM Chris Plummer <chris.plummer at oracle.com>
wrote:
> Hi JC,
>
> I went to check the change I suggested in nativemethbind003.cpp and found
> another line with a \n in the middle:
>
> 129 NSK_COMPLAIN5(
> 130 "TEST FAILED: wrong NativeMethodBind events\n\tfor tested
> method \"%s %s\" bound with \"%s\":\n"
> 131 "\tgot: %d\texpected: %d\n\n",
>
> Also ap01t001.cpp has the same missing \n that ap01t012.cpp had:
>
> 69 NSK_COMPLAIN2(
> 70 "Received unexpected number of ObjectFree events: %d\n"
> 71 "\texpected number: %d",
> 72 obj_free, (EXP_OBJ_NUMBER - 1));
>
> Otherwise looks good. I don't need to see another review.
>
> thanks,
>
> Chris
>
> On 9/24/18 9:16 AM, JC Beyler wrote:
>
> 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
>
>
>
--
Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180924/37f2ec1b/attachment-0001.html>
More information about the serviceability-dev
mailing list