[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR (M) 8210689: Remove the multi-line old C style for string literals
From: Chris Plummer <chris.plummer () oracle ! com>
Date: 2018-09-24 22:37:11
Message-ID: ce845603-34c5-ebe4-ad74-138ca6d93193 () oracle ! com
[Download RAW message or body]
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Looks good!<br>
<br>
Chris<br>
<br>
On 9/24/18 2:30 PM, JC Beyler wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAF9BGBy8QsrCqUxnZ54SdfW0xd0a08EPDc+omo8tjhnWN_iRtA@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<div dir="ltr">
<div dir="ltr">Thanks Chris!
<div><br>
</div>
<div>Because you saw that, I double checked and actually saw a
few more so I'm just showing the incremental here for your
information:</div>
<div><a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210689/webrev.03_04/"
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8210689/webrev.03_04/</a><br>
</div>
<div><br>
</div>
<div>(The full is here: <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210689/webrev.04/"
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8210689/webrev.04/</a>)</div>
<div><br>
</div>
<div>I've restarted a manual test and will submit if it
passes.</div>
<div><br>
</div>
<div>Thanks for the review!</div>
<div>Jc</div>
<div><br>
</div>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Mon, Sep 24, 2018 at 10:52 AM Chris Plummer
<<a href="mailto:chris.plummer@oracle.com" target="_blank"
moz-do-not-send="true">chris.plummer@oracle.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF">
<div
class="m_-7449580839113583974m_-1894986536376843335moz-cite-prefix">Hi
JC,<br>
<br>
I went to check the change I suggested in
nativemethbind003.cpp and found another line with a \n in
the middle:<br>
<br>
129 NSK_COMPLAIN5(<br>
130 "TEST FAILED: wrong NativeMethodBind
events\n\tfor tested method \"%s %s\" bound with
\"%s\":\n"<br>
131 "\tgot: %d\texpected: %d\n\n",<br>
<br>
Also ap01t001.cpp has the same missing \n that
ap01t012.cpp had:<br>
<br>
69 NSK_COMPLAIN2(<br>
70 "Received unexpected number of ObjectFree
events: %d\n"<br>
71 "\texpected number: %d",<br>
72 obj_free, (EXP_OBJ_NUMBER - 1));<br>
<br>
Otherwise looks good. I don't need to see another review.<br>
<br>
thanks,<br>
<br>
Chris<br>
<br>
On 9/24/18 9:16 AM, JC Beyler wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">Thanks Alex!
<div><br>
</div>
<div>Could I get a second review/LGTM ?</div>
<div><br>
</div>
<div>Thanks for your help!</div>
<div>Jc</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Fri, Sep 21, 2018 at 5:22 PM Alex
Menkov <<a href="mailto:alexey.menkov@oracle.com"
target="_blank" \
moz-do-not-send="true">alexey.menkov@oracle.com</a>> wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">LGTM.<br>
<br>
--alex<br>
<br>
On 09/21/2018 17:06, JC Beyler wrote:<br>
> Hi Alex,<br>
> <br>
> Good catch, it was not done on purpose but now
fixed:<br>
> <br>
> Webrev: <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210689/webrev.03/"
rel="noreferrer" target="_blank"
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8210689/webrev.03/</a>
<br>
> <<a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210689/webrev.03/"
rel="noreferrer" target="_blank"
moz-do-not-send="true">http://cr.openjdk.java.net/%7Ejcbeyler/8210689/webrev.03/</a>><br>
> Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8210689"
rel="noreferrer" target="_blank"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8210689</a><br>
> <br>
> Let me know if this works for you and thanks for
the review,<br>
> Jc<br>
> <br>
> On Fri, Sep 21, 2018 at 3:44 PM Alex Menkov <<a
href="mailto:alexey.menkov@oracle.com"
target="_blank" \
moz-do-not-send="true">alexey.menkov@oracle.com</a> <br>
> <mailto:<a
href="mailto:alexey.menkov@oracle.com"
target="_blank" \
moz-do-not-send="true">alexey.menkov@oracle.com</a>>> wrote:<br>
> <br>
> Hi Jc,<br>
> <br>
> overall looks good (no changes in the
logging)<br>
> except<br>
>
test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t003/hs201t003.cpp<br>
> <br>
> :<br>
> - if ((strcmp(name, expMeth) == 0)
&&<br>
> - (strcmp(sig, expSig) == 0)) {<br>
> - NSK_DISPLAY4("===== %s event
received for the tested method:\n\<br>
> -\tID=0x%p name=\"%s\" signature=\"%s\"\n",<br>
> + if ((strcmp(name, expMeth) == 0)
&& (strcmp(sig, expSig) == 0)) {<br>
> + NSK_DISPLAY4(<br>
> + "%s event received for the
tested method:\n"<br>
> + "\tID=0x%p name=\"%s\"
signature=\"%s\"\n",<br>
> <br>
> "===== " is dropped from the beginning of the
line<br>
> I don't know if this is important.<br>
> <br>
> --alex<br>
> <br>
> <br>
> On 09/21/2018 14:29, JC Beyler wrote:<br>
> > Hi Chris,<br>
> ><br>
> > Done here:<br>
> > Webrev: <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210689/webrev.02/"
rel="noreferrer" target="_blank"
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8210689/webrev.02/</a><br>
> <<a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210689/webrev.02/"
rel="noreferrer" target="_blank"
moz-do-not-send="true">http://cr.openjdk.java.net/%7Ejcbeyler/8210689/webrev.02/</a>><br>
> > <<a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210689/webrev.02/"
rel="noreferrer" target="_blank"
moz-do-not-send="true">http://cr.openjdk.java.net/%7Ejcbeyler/8210689/webrev.02/</a>><br>
> > Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8210689"
rel="noreferrer" target="_blank"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8210689</a><br>
> ><br>
> > Anything else? and anybody else
motivated to look?<br>
> ><br>
> > Thanks again!<br>
> > Jc<br>
> ><br>
> > On Fri, Sep 21, 2018 at 2:07 PM Chris
Plummer<br>
> <<a href="mailto:chris.plummer@oracle.com"
target="_blank" \
moz-do-not-send="true">chris.plummer@oracle.com</a> <mailto:<a \
href="mailto:chris.plummer@oracle.com"
target="_blank" \
moz-do-not-send="true">chris.plummer@oracle.com</a>><br> > > \
<mailto:<a href="mailto:chris.plummer@oracle.com"
target="_blank" \
moz-do-not-send="true">chris.plummer@oracle.com</a><br> > <mailto:<a
href="mailto:chris.plummer@oracle.com"
target="_blank" \
moz-do-not-send="true">chris.plummer@oracle.com</a>>>> wrote:<br>
> ><br>
> > Hi JC,<br>
> ><br>
> > Overall looks good. Just a couple
minor edits needed:<br>
> ><br>
> > In nativemethbind003.cpp:<br>
> ><br>
> > 158 NSK_DISPLAY1("Inside the
registerNative()\nFinding<br>
> class<br>
> > \"%s\" ...\n", CLASS_SIG);<br>
> ><br>
> > This was two lines and you made it
one with a \n in the middle of<br>
> > the string.<br>
> ><br>
> > In ap12t001.cpp:<br>
> ><br>
> > 69 NSK_COMPLAIN2(<br>
> > 70 "Received
unexpected number of ObjectFree<br>
> events:<br>
> > %d\n"<br>
> > 71 "\texpected
number: %d",<br>
> > 72 obj_free,
EXP_OBJ_FREE);<br>
> ><br>
> > There's no \n at the end of this
output (and there never was).<br>
> > Normally NSK_COMPLAIN is always
used with a terminating \n.<br>
> ><br>
> > thanks,<br>
> ><br>
> > Chris<br>
> ><br>
> ><br>
> > On 9/21/18 1:05 PM, JC Beyler
wrote:<br>
> >> Hi Chris,<br>
> >><br>
> >> Sounds good to me; here it is:<br>
> >> Webrev:<br>
> <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210689/webrev.01/"
rel="noreferrer" target="_blank"
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8210689/webrev.01/</a><br>
> <<a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210689/webrev.01/"
rel="noreferrer" target="_blank"
moz-do-not-send="true">http://cr.openjdk.java.net/%7Ejcbeyler/8210689/webrev.01/</a>><br>
> >> <<a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210689/webrev.01/"
rel="noreferrer" target="_blank"
moz-do-not-send="true">http://cr.openjdk.java.net/%7Ejcbeyler/8210689/webrev.01/</a>><br>
> >> Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8210689"
rel="noreferrer" target="_blank"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8210689</a><br>
> >><br>
> >> I admit I strived to stay
consistent and always started a<br>
> new line<br>
> >> for the multi-line argument
even if the string was not too long;<br>
> >> it's a question of style I
believe but it felt more readable to<br>
> >> me. I'll happily change
whatever anyone prefers.<br>
> >><br>
> >> This has passed the vmTestbase
tests I changed but due to the<br>
> >> shared changes, I've launched a
full vmTestbase testing now.<br>
> >><br>
> >> Let me know what you think,<br>
> >> Jc<br>
> >><br>
> >> On Fri, Sep 21, 2018 at 10:59
AM Chris Plummer<br>
> >> <<a
href="mailto:chris.plummer@oracle.com"
target="_blank" \
moz-do-not-send="true">chris.plummer@oracle.com</a> <mailto:<a \
href="mailto:chris.plummer@oracle.com"
target="_blank" \
moz-do-not-send="true">chris.plummer@oracle.com</a>><br> > \
<mailto:<a href="mailto:chris.plummer@oracle.com"
target="_blank" \
moz-do-not-send="true">chris.plummer@oracle.com</a> <mailto:<a \
href="mailto:chris.plummer@oracle.com"
target="_blank" \
moz-do-not-send="true">chris.plummer@oracle.com</a>>>><br> > \
wrote:<br> > >><br>
> >> On 9/21/18 10:55 AM, JC
Beyler wrote:<br>
> >>> Hi Chris,<br>
> >>><br>
> >>> I hesitated to be
honest and then thought that<br>
> debug_str was<br>
> >>> better as you would
clearly see that it is a multi-lilne<br>
> >>> string and what
parameters are what. But I'll take your<br>
> >>> preference (it's
relatively the same for me). Quick<br>
> question<br>
> >>> though:<br>
> >>><br>
> >>> Do you have a
preference between:<br>
> >>>
NSK_COMPLAIN6(<br>
> >>> \
"TEST FAILED: %s field \"%s\" has\n"<br>
> >>>
"\tsignature: \"%s\"\n"<br>
> >>>
"\tgeneric signature: \"%s\"\n\n"<br>
> >>>
"\tExpected: \"%s\"\n"<br>
> >>>
"\t\t\"%s\"\n\n",<br>
> >>>
(instance==0)?"instance":"static",<br>
> >>>
fld_sig[idx][0],<br>
> >>> \
sign, (gen_sign==NULL)?"NULL":gen_sign,<br>
> >>>
fld_sig[idx][2], fld_sig[idx][3]);<br>
> >>> or:<br>
> >>>
NSK_COMPLAIN6(<br>
> >>> \
"TEST FAILED: %s field \"%s\"<br>
> has\n\tsignature: \"%s\"\n"<br>
> >>>
"\tgeneric signature:<br>
> \"%s\"\n\n\tExpected:
\"%s\"\n\t\t\"%s\"\n\n",<br>
> >>>
(instance==0)?"instance":"static",<br>
> >>>
fld_sig[idx][0],<br>
> >>> \
sign, (gen_sign==NULL)?"NULL":gen_sign,<br>
> >>>
fld_sig[idx][2], fld_sig[idx][3]);<br>
> >>> I think I like the
first because you can clearly see<br>
> what we want to be printed out; but for code
vertical<br>
> >>> compression, the second
is better. What do you think?<br>
> >> I also prefer the first
one.<br>
> >><br>
> >> thanks,<br>
> >><br>
> >> Chris<br>
> >>> Thanks!<br>
> >>> Jc<br>
> >>><br>
> >>> On Fri, Sep 21, 2018 at
10:16 AM Chris Plummer<br>
> >>> <<a
href="mailto:chris.plummer@oracle.com"
target="_blank" \
moz-do-not-send="true">chris.plummer@oracle.com</a><br> > <mailto:<a
href="mailto:chris.plummer@oracle.com"
target="_blank" \
moz-do-not-send="true">chris.plummer@oracle.com</a>> <mailto:<a \
href="mailto:chris.plummer@oracle.com"
target="_blank" \
moz-do-not-send="true">chris.plummer@oracle.com</a><br> > <mailto:<a
href="mailto:chris.plummer@oracle.com"
target="_blank" \
moz-do-not-send="true">chris.plummer@oracle.com</a>>>><br> > \
>>> wrote:<br> > >>><br>
> >>> Hi JC,<br>
> >>><br>
> >>> I didn't realize
you intended to move all the strings<br>
> >>> into a "const
char*" first. Seems unnecessary, and I<br>
> >>> think not as easy
to read:<br>
> >>><br>
> >>> 138 \
const char* debug_str =<br>
> >>> 139 \
"TEST FAILED: JVMTI_EVENT_CLASS_LOAD<br>
> >>> event received
for\n"<br>
> >>> 140 \
"\t a primitive class/array of<br>
> primitive<br>
> >>> types with the
signature \"%s\"\n";<br>
> >>> 141
NSK_COMPLAIN1(debug_str, sig);<br>
> >>><br>
> >>> vs<br>
> >>><br>
> >>> 138
NSK_COMPLAIN1("TEST FAILED:<br>
> >>>
JVMTI_EVENT_CLASS_LOAD event received for\n"<br>
> >>>
139 "\t a \
primitive<br> > class/array of<br>
> >>> primitive types
with the signature \"%s\"\n",<br>
> >>>
140 sig);<br>
> >>><br>
> >>> or<br>
> >>><br>
> >>> 138
NSK_COMPLAIN1(<br>
> >>> 139 \
"TEST FAILED: JVMTI_EVENT_CLASS_LOAD<br>
> >>> event received
for\n"<br>
> >>> 140 \
"\t a primitive class/array of<br>
> primitive<br>
> >>> types with the
signature \"%s\"\n",<br>
> >>> 141 \
sig);<br>
> >>><br>
> >>> thanks,<br>
> >>><br>
> >>> Chris<br>
> >>><br>
> >>> On 9/21/18 8:00 AM,
JC Beyler wrote:<br>
> >>>> Hi all,<br>
> >>>><br>
> >>>> Is anyone
motivated on a Friday to review this ? :)<br>
> >>>><br>
> >>>> It should be
fairly straightforward :-)<br>
> >>>><br>
> >>>> Thanks,<br>
> >>>> Jc<br>
> >>>><br>
> >>>> On Tue, Sep 18,
2018 at 12:07 PM JC Beyler<br>
> >>>> <<a
href="mailto:jcbeyler@google.com" target="_blank"
moz-do-not-send="true">jcbeyler@google.com</a>
<mailto:<a href="mailto:jcbeyler@google.com"
target="_blank" \
moz-do-not-send="true">jcbeyler@google.com</a>><br> > <mailto:<a
href="mailto:jcbeyler@google.com" target="_blank"
moz-do-not-send="true">jcbeyler@google.com</a>
<mailto:<a href="mailto:jcbeyler@google.com"
target="_blank" \
moz-do-not-send="true">jcbeyler@google.com</a>>>> wrote:<br>
> >>>><br>
> >>>> Hi all,<br>
> >>>><br>
> >>>> Could I
have a review for this webrev:<br>
> >>>><br>
> >>>> Webrev:<br>
> >>>> <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210689/webrev.00/"
rel="noreferrer" target="_blank"
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8210689/webrev.00/</a><br>
> <<a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210689/webrev.00/"
rel="noreferrer" target="_blank"
moz-do-not-send="true">http://cr.openjdk.java.net/%7Ejcbeyler/8210689/webrev.00/</a>><br>
> >>>> <br>
> <<a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210689/webrev.00/"
rel="noreferrer" target="_blank"
moz-do-not-send="true">http://cr.openjdk.java.net/%7Ejcbeyler/8210689/webrev.00/</a>><br>
> >>>> Bug:<br>
> <a
href="https://bugs.openjdk.java.net/browse/JDK-8210689"
rel="noreferrer" target="_blank"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8210689</a><br>
> >>>><br>
> >>>> Let me know
what you think,<br>
> >>>> Jc<br>
> >>>><br>
> >>>><br>
> >>>><br>
> >>>> --<br>
> >>>><br>
> >>>> Thanks,<br>
> >>>> Jc<br>
> >>><br>
> >>><br>
> >>><br>
> >>><br>
> >>> --<br>
> >>><br>
> >>> Thanks,<br>
> >>> Jc<br>
> >><br>
> >><br>
> >><br>
> >><br>
> >> --<br>
> >><br>
> >> Thanks,<br>
> >> Jc<br>
> ><br>
> ><br>
> ><br>
> ><br>
> > --<br>
> ><br>
> > Thanks,<br>
> > Jc<br>
> <br>
> <br>
> <br>
> -- <br>
> <br>
> Thanks,<br>
> Jc<br>
</blockquote>
</div>
<br clear="all">
<div><br>
</div>
-- <br>
<div dir="ltr"
class="m_-7449580839113583974m_-1894986536376843335gmail_signature"
data-smartmail="gmail_signature">
<div dir="ltr">
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</blockquote>
<p><br>
</p>
</div>
</blockquote>
</div>
<br clear="all">
<div><br>
</div>
-- <br>
<div dir="ltr" class="m_-7449580839113583974gmail_signature"
data-smartmail="gmail_signature">
<div dir="ltr">
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</blockquote>
<p><br>
</p>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic