Hi Chris,
Done here:
Webrev: http://cr.openjdk.java.net/~jcbeyler/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>
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/
> 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>
> 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>
>> 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> wrote:
>>>
>>>> Hi all,
>>>>
>>>> Could I have a review for this webrev:
>>>>
>>>> Webrev: http://cr.openjdk.java.net/~jcbeyler/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
