On 10/31/18 11:30 AM, serguei.spit...@oracle.com wrote:
It prints the FILE/LINE which is enough to identify the error point.
Yes, but it also prints the JNI calls.
But I agree that doing the assignments within the NSK_JNI_VERIFY was
intentional as it gives more context.
From the other hand it make the code ugly and less readable.
Not sure, what direction is better to go.
Agreed. Somewhat of a tossup now as to whether or not this change should
be done. I'm fine either way.
Chris
Thanks,
Serguei
On 10/31/18 11:21, Chris Plummer wrote:
Hi Claes,
It's good that you brought this up. NSK_JNI_VERIFY is always "on",
but moving the assignment outside of it does slightly change the
behavior of the macro (although the code still executes "correctly"):
/**
* Execute action with JNI call, check result for true and
* pending exception and complain error if required.
* Also trace action execution if tracing mode enabled.
*/
#define NSK_JNI_VERIFY(jni, action) \
(nsk_ltrace(NSK_TRACE_BEFORE,__FILE__,__LINE__,"%s\n",#action), \
nsk_jni_lverify(NSK_TRUE,jni,action,__FILE__,__LINE__,"%s\n",#action))
So you will no longer see the JNI call in the trace or error output.
You will just see the comparison to the JNI result, which gives no
context as to the source of the result. So I'm now starting to think
that doing the assignments within the NSK_JNI_VERIFY was intentional
so we would see the JNI call in the trace output.
Chris
On 10/31/18 3:16 AM, Claes Redestad wrote:
Hi,
here's a case that your script seems to miss:
http://cr.openjdk.java.net/%7Ejcbeyler/8213003/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassLoaderClasses/clsldrclss002/clsldrclss002.cpp.udiff.html
if (!NSK_JNI_VERIFY(jni, (testedFieldID =
jni->GetStaticFieldID(testedClass, FIELD_NAME,
FIELD_SIGNATURE)) != NULL))
I'd note that with some of your changes here you're unconditionally
executing logic outside of NSK_JNI_VERIFY macros. Does care need be
taken to wrap the code blocks in equivalent macros/ifdefs? Or are
the NSK_JNI_VERIFY macros always-on in these tests (and essentially
pointless)?
/Claes
On 2018-10-31 05:42, JC Beyler wrote:
Hi all,
I worked on updating my script and handling more assignments so I
redid a second pass on the same files to get all the cases. Now, on
those files the regex "if.* = " no longer shows any cases we would
want to fix.
Could I get a review for this webrev:
Webrev: http://cr.openjdk.java.net/~jcbeyler/8213003/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8213003
I tested this on my dev machine by passing all the tests that were
modified.
Thanks!
Jc
