|
Looks good!
Chris
On 9/24/18 2:30 PM, JC Beyler wrote:
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:
I've restarted a manual test and will submit if it
passes.
Thanks for the review!
Jc
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
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
--
--
|
