Hi Jc,
It looks great to me.
Thank you for the update!
Thanks,
Serguei
On 5/6/19 17:37, Jean Christophe Beyler wrote:
Hi both,
I think it all makes sense so I did this:
@Serguei: I think since I am changing the call sites
anyway, it makes sense to just change it to ec_jni for all; so
I went ahead and did it retro-actively to all call sites
@Chris: I am not shocked or my eyes are not crying by
seeing TRACE_JNI_CALL_VARARGS so I went ahead and did that as
well; I think it works out better too:
You both said "leave it up to you" but I'd rather wait for
a final LGTM from both (and/or reviews from others) of you and
I can move forward, I'll re-submit for testing via the submit
repo (all the affected tests pass on my dev machine already).
Thanks again,
Jc
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.
--
--
--
--
--
--
--
|