|
Hi JC,
After looking at the following:
71 methodID = jni_env->GetMethodID(
72 klass, "loadClass",
"(Ljava/lang/String;)Ljava/lang/Class;", TRACE_JNI_CALL);
73 loadedClass = (jclass)
jni_env->CallObjectMethod(loader, methodID,
TRACE_VARARGS(className));
I was wondering if the naming of TRACE_VARARGS should more closely
resemble TRACE_JNI_CALL. Maybe TRACE_JNI_VARARGS or
TRACE_JNI_CALL_VARARGS (starting to get a bit wordy though)?
I'll leave it up to you. Other than that it looks fine.
thanks,
Chris
On 5/4/19 2:12 PM, Jean Christophe Beyler wrote:
Hi Chris and Serguei,
Then the new webrev is here:
Let me know if now it looks good for you;
@Serguei, you had said it looks good but had comments;
you never answered my comment about naming; would you like
me to rename all the environments to ec_jni_env? I'd do
another webrev for the ones not done in this webrev but that
are already under the ExceptionCheckingJniEnvPtr, let me
know :)
Finally, does anyone else have comments?
Thanks,
Jc
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.
--
--
--
--
--
--
|
