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