|
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:
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 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
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
Hi all,
Could I have a review for
this webrev:
Let me know what you think,
--
--
--
|
