Re: RFR (L) 8228998: Remove the testing against NSK_FALSE from tests

[serguei . spitsyn]

Hi Jc,

Looks good.

This still aligned incorrectly:

221 static bool getFieldsAndObjects(jvmtiEnv* jvmti,
222 JNIEnv* jni,
223 jclass debugeeClass,
224 jclass rootObjectClass,
225 jclass chainObjectClass,
226 jobject* rootObjectPtr,
227 jfieldID* reachableChainField,
228 jfieldID* unreachableChainField,
229 jfieldID* nextField) {


Some copyright comments need an update.
No need in another review if you fix it.
You may want to update the webrev in place for other reviewers.

Thanks,
Serguei


On 8/1/19 4:53 PM, Jean Christophe Beyler wrote:

Hi Serguei,

My apologies. I fixed the forbidden on the old webrev link. Then I rechecked the white-spaces, and made webrev not ignore white space changes. Here is the new webrev:

Webrev: http://cr.openjdk.java.net/~jcbeyler/8228998/webrev.01/ <http://cr.openjdk.java.net/%7Ejcbeyler/8228998/webrev.01/>

Bug: https://bugs.openjdk.java.net/browse/JDK-8228998

Thanks for your review :)
Jc

On Thu, Aug 1, 2019 at 4:07 PM <serguei.spit...@oracle.com <mailto:serguei.spit...@oracle.com>> wrote:

    Hi Jc,

    Thank you for taking care about this!
    Most of the links in the webrev can not be resolved.
    I'm getting the error: "403 - Forbidden".

    The only item that works is:

    |Cdiffs
    
<http://cr.openjdk.java.net/%7Ejcbeyler/8228998/webrev/test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/FollowReferences/followref001/followref001.cpp.cdiff.html>
    Udiffs
    
<http://cr.openjdk.java.net/%7Ejcbeyler/8228998/webrev/test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/FollowReferences/followref001/followref001.cpp.udiff.html>
    Sdiffs
    
<http://cr.openjdk.java.net/%7Ejcbeyler/8228998/webrev/test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/FollowReferences/followref001/followref001.cpp.sdiff.html>
    Frames
    
<http://cr.openjdk.java.net/%7Ejcbeyler/8228998/webrev/test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/FollowReferences/followref001/followref001.cpp.frames.html>
    Old
    
<http://cr.openjdk.java.net/%7Ejcbeyler/8228998/webrev/test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/FollowReferences/followref001/followref001.cpp-.html>
    New
    
<http://cr.openjdk.java.net/%7Ejcbeyler/8228998/webrev/test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/FollowReferences/followref001/followref001.cpp.html>
    ----- Raw
    
<http://cr.openjdk.java.net/%7Ejcbeyler/8228998/webrev/raw_files/new/test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/FollowReferences/followref001/followref001.cpp>

    |
    
*test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/FollowReferences/followref001/followref001.cpp*


    Also, the patch is readable:
    http://cr.openjdk.java.net/%7Ejcbeyler/8228998/webrev/jdk-false.changeset


    It looks pretty good.

    Only a one comments:

    
http://cr.openjdk.java.net/%7Ejcbeyler/8228998/webrev/test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/FollowReferences/followref001/followref001.cpp.frames.html

    A couple of fragments are not aligned properly:

    221 static bool getFieldsAndObjects(jvmtiEnv* jvmti,
      222                                 JNIEnv*     jni,
      223                                 jclass      debugeeClass,
      . . .

    434 static bool checkTestedObjects(jvmtiEnv* jvmti,
      435                               JNIEnv*    jni,
      436                               int        chainLength,
      437                               ObjectDesc objectDescList[])


    Thanks,
    Serguei


    On 8/1/19 3:16 PM, Jean Christophe Beyler wrote:

    Hi all,

    It took me a while to pick this item back but here we go :-).
    Here is a webrev that removes all the if (.* == NSK_FALSE) and
    replaces them with if (! .*).

    Webrev: http://cr.openjdk.java.net/~jcbeyler/8228998/webrev/
    <http://cr.openjdk.java.net/%7Ejcbeyler/8228998/webrev/>
    Bug: https://bugs.openjdk.java.net/browse/JDK-8228998

    For the EM tests, I also updated the returns to be boolean,
    entirely removing the NSK_FALSE/NSK_TRUE parts because of the way
    the tests were done. Let me know if you'd rather I divide those up.

    This was tested by running the tests changed on my dev machine,
    I'll push it to the submit repo after review :-)

    Thanks and have a great evening,
    Jc

--

Thanks,
Jc