|
On 4/30/19 3:36 PM, Jean Christophe
Beyler wrote:
You are right I was overthinking the naming, so
I did this now:
- loadedClass = (jclass)
jni_env->CallObjectMethod(loader, methodID,
TRACE_JNI_CALL, className);
+ loadedClass = (jclass)
jni_env->CallObjectMethod(loader, methodID,
TRACE_VARARGS(className));
Ok. I like this.
Question now is: this variadic won't work if there are
no arguments in a portable way, so if there are no
arguments for the call, should we use a TRACE_JNI_CALL
such as:
jni_env->CallVoidMethod(thread, methodID,
TRACE_JNI_CALL);
or should I create a TRACE_VARARGS for no arguments:
jni_env->CallVoidMethod(thread, methodID,
TRACE_EMPTY_VARARGS());
Advantage of the latter is that you would know it is a
VARARG at least and you can see it; draw-back is yet
another macro.
In general we don't know it is vararg when looking at a callsite, so
I don't feel the need to advertise it as such with
TRACE_EMPTY_VARARGS(). I'd recommend just directly passing
TRACE_JNI_CALL.
Chris
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.
--
--
--
--
--
|
