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.men...@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.men...@oracle.com > > <mailto:alexey.men...@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.plum...@oracle.com <mailto:chris.plum...@oracle.com> > > > <mailto:chris.plum...@oracle.com > > <mailto:chris.plum...@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.plum...@oracle.com <mailto:chris.plum...@oracle.com> > > <mailto:chris.plum...@oracle.com <mailto:chris.plum...@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.plum...@oracle.com > > <mailto:chris.plum...@oracle.com> <mailto:chris.plum...@oracle.com > > <mailto:chris.plum...@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 > > >>>> <jcbey...@google.com <mailto:jcbey...@google.com> > > <mailto:jcbey...@google.com <mailto:jcbey...@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
