RFR (M) 8223044: Add back exception checking in tests

[Jean Christophe Beyler]

I do have more of these coming and there are 86 CallXMethod cases where
this will occur and 49 NewObject cases. So it would be good to figure out
something that does not hurt our eyes too many times.

I think I would go with the TRACED_VARARGS, it at least has the advantage
of not being too bad. I would move toward doing:
 73     loadedClass = (jclass) jni_env->CallObjectMethod(loader, methodID,
TRACED_JNI_CALL(className));

To be consisted with the non-vararg cases, what do you think?
Jc

On Tue, Apr 30, 2019 at 9:51 AM Chris Plummer <chris.plummer at oracle.com>
wrote:

> The advantage of the TRACED_VARARGS approach (or leaving it as-is) is that
> there are limited APIs and callsites affected (only 1 of each in this
> review, but it's unclear to me if you have more of these changes coming).
>
> Chris
>
> On 4/29/19 9:49 PM, Jean Christophe Beyler wrote:
>
> Yes I would fix them all in the next webrev and then continue the porting.
> I 100% agree to which is less offensive. I find that
>
>  73     loadedClass = (jclass) jni_env->CALLOBJECTMETHODTRACED(loader,
> methodID, className);
>
> to be offensive as well.
>
> So perhaps the TRACED_VARARGS is best:
>
>
>  73     loadedClass = (jclass) jni_env->CallObjectMethod(loader, methodID,
> TRACED_VARARGS(className));
>
> What do you think?
>
>
> On Mon, Apr 29, 2019 at 9:44 PM Chris Plummer <chris.plummer at oracle.com>
> wrote:
>
>> Hi JC,
>>
>> On 4/29/19 4:25 PM, Jean Christophe Beyler wrote:
>>
>> Hi Chris,
>>
>> I agree it's not ideal, how about putting it first?
>>
>>   73     loadedClass = (jclass)
>> jni_env->CallObjectMethod(TRACE_JNI_CALL, loader, methodID, className);
>>
>> It's not consistent with other uses, or are you suggesting changing them
>> all?
>>
>>
>> I find that less awkward than the VAR_ARGS, no?
>>
>> Or something like:
>>  73     loadedClass = (jclass) jni_env->CallObjectMethodTraced(loader,
>> methodID, className);
>>
>> Where CallObjectMethodTraced is actually a define in the exception
>> handling system that adds the line and filename...
>>
>> I don't like macroizing a method call in this manner (macros should be
>> all upper case, right?). Also it's inconsistent with the other tracing
>> calls, unless you plan on doing it to all of them.
>>
>>
>> Which looks better?
>>
>> Not sure. I wouldn't ask which is better. I'd ask which is less
>> offensive. :)
>>
>> thanks,
>>
>> Chris
>>
>>
>> Thanks again,
>> Jc
>>
>> On Mon, Apr 29, 2019 at 3:40 PM Chris Plummer <chris.plummer at oracle.com>
>> wrote:
>>
>>> Ok. I only half heatedly suggest the following
>>>
>>>   73     loadedClass = (jclass) jni_env->CallObjectMethod(loader,
>>> methodID, VAR_ARGS(className));
>>>
>>> And then VAR_ARGS can contain the reference to TRACE_JNI_CALL.
>>>
>>> Chris
>>>
>>> On 4/29/19 2:58 PM, Jean Christophe Beyler wrote:
>>>
>>> Hi Chris,
>>>
>>> Thanks for the review! It is the only issue I have with the system now
>>> and I had not found a good solution for:
>>>
>>> as CallObjectMethod is a variadic method, I can't put TRACE_JNI_CALL at
>>> the end; so I put right before the end; that allows me to do this:
>>>
>>>
>>> +jobject ExceptionCheckingJniEnv::CallObjectMethod(jobject obj, jmethodID methodID, int line,+                         const char* file_name, ...) {+  JNIVerifier<> marker(this, "CallObjectMethod", obj, methodID, line, file_name);++  va_list args;+  va_start(args, file_name);+  jobject result = _jni_env->CallObjectMethodV(obj, methodID, args);+  va_end(args);+  return result;+}
>>>
>>> Putting it at the end would mean I would have to play with the va_list to remove the last one, and from what I have understood with va_list, it's kind of a no-no to do so. So I left at this.
>>>
>>> Do you have any better suggestion?
>>>
>>> Thanks!
>>>
>>> Jc
>>>
>>>
>>> On Mon, Apr 29, 2019 at 10:38 AM Chris Plummer <chris.plummer at oracle.com>
>>> wrote:
>>>
>>>> Hi JC,
>>>>
>>>> In em01t002.cpp, is this correct?
>>>>
>>>>   73     loadedClass = (jclass) jni_env->CallObjectMethod(loader,
>>>> methodID, TRACE_JNI_CALL, className);
>>>>
>>>> Shouldn't the TRACE_JNI_CALL arg be last? If so, can you look into why
>>>> this test didn't fail as a result.
>>>>
>>>> Other than that the changes look good.
>>>>
>>>> thanks,
>>>>
>>>> Chris
>>>>
>>>> On 4/26/19 5:52 PM, Jean Christophe Beyler wrote:
>>>>
>>>> Hi all,
>>>>
>>>> (Re-sending with subject line complete :-))
>>>>
>>>> Since JDK-8213501 finally merged (sorry it took so long), I am able to
>>>> continue this work. Here is the work that puts back the messages for any
>>>> calls that were moved around due to JDK-8212884.
>>>>
>>>> Webrev: http://cr.openjdk.java.net/~jcbeyler/8223044/webrev.00/
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8223044
>>>>
>>>> All modified tests pass on my local dev machine.
>>>>
>>>> Thanks,
>>>> Jc
>>>>
>>>>
>>>>
>>>
>>> --
>>>
>>> Thanks,
>>> Jc
>>>
>>>
>>>
>>
>> --
>>
>> Thanks,
>> Jc
>>
>>
>>
>
> --
>
> Thanks,
> Jc
>
>
>

-- 

Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190430/57528089/attachment.html>