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

[Chris Plummer]

On 10/31/18 11:30 AM, serguei.spitsyn at 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
>>>
>>
>>
>