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.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/>
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>> 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>>
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>> 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
