RFR (M) 8213003: Remove more assignments in ifs for vmTestbase/[a-s]

[Chris Plummer]

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
>