|
Hi JC,
I'm not sure why you are suggesting TRACED_JNI_CALL instead of
TRACED_VARARGS. How are they different? Are you suggesting
non-varargs calls would just end up using an empty
TRACED_JNI_CALL()?
Chris
On 4/30/19 10:05 AM, Jean Christophe Beyler wrote:
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?
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?
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.
Not sure. I wouldn't ask which is better. I'd ask
which is less offensive. :)
thanks,
Chris
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
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.
All modified tests pass
on my local dev machine.
--
--
--
--
|
