RFR(S): 8165433: Convert Test_linked_list to Gtest

[Kirill Zhaldybin]

Igor,

Thank you for review!

Regards, Kirill

On 15.09.2016 20:10, Igor Ignatyev wrote:
> Kirill,
>
> looks good to me.
>
> Thanks,
> — Igor
>
>> On Sep 12, 2016, at 3:30 PM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
>>
>> This looks good.
>> Coleen
>>
>>
>> On 9/12/16 8:16 AM, Kirill Zhaldbybin wrote:
>>> David,
>>>
>>> Thank you for review!
>>>
>>> Regards, Kirill
>>>
>>> On 09/09/2016 08:03 AM, David Holmes wrote:
>>>> On 8/09/2016 10:32 PM, Kirill Zhaldybin wrote:
>>>>> Igor,
>>>>>
>>>>> Thank you for clarifying this!
>>>> Indeed! Thank you very much.
>>>>
>>>>> David,
>>>>>
>>>>> Here are a new WebRev:
>>>>> http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8165433/webrev.03/
>>>>>
>>>>> I changed asserts like ASSERT_NE(i, (Integer*) NULL) to ASSERT_TRUE,
>>>>> changed order in ASSERT_EQ with NULL, changed order in ASSERT_EQ to make
>>>>> expected value first parameter.
>>>>>
>>>>> Could you please let me know your opinion?
>>>> Looks terrific! :)
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Thank you.
>>>>>
>>>>> Regards, Kirill
>>>>>
>>>>> On 08.09.2016 12:22, Igor Ignatyev wrote:
>>>>>> David,
>>>>>>
>>>>>> I ain’t Kim, but if you don’t mind I’d like to say what I see as the
>>>>>> best way to handle that is and why.
>>>>>>
>>>>>> as Kirill said gtest’s asserts are strict about types and one will get
>>>>>> compile time error trying to compare different types, long and pointer
>>>>>> in this case (as the matter of fact, you can not compare pointers to
>>>>>> different types either). however gtest supports comparison w/ NULL,
>>>>>> but only in ASSERT/EXPECT_EQ and only if NULL is the 1st argument[1].
>>>>>> so you can easily write ASSERT_EQ(NULL, i), but you can not write
>>>>>> ASSERT_NE(NULL, i), you have to write ASSERT_NE(i != NULL) instead,
>>>>>> [2] is the explanation from gtest faq why it so, in two words: in case
>>>>>> ASSERT_NE(i, NULL) fails, you do know the value of i so ASSERT_TRUE(i
>>>>>> != NULL) won’t lose any information.
>>>>>>
>>>>>> summing up, Kirill’s code can be changed like that:
>>>>>>> diff -r 28f34e9482b4 test/native/utilities/test_linkedlist.cpp
>>>>>>>     Integer* i = ll.find(six);
>>>>>>> -  ASSERT_NE(i, (Integer*) NULL) << "Should find it";
>>>>>>> +  ASSERT_TRUE(i != NULL) << "Should find it";
>>>>>>>     ASSERT_EQ(i->value(), six.value()) << "Should be 6";
>>>>>>>       i = ll.find(three);
>>>>>>> -  ASSERT_EQ(i, (Integer*) NULL) << "Not in the list";
>>>>>>> +  ASSERT_EQ(NULL, i) << "Not in the list";
>>>>>> as you can see, there is no casting.
>>>>>>
>>>>>> could you please let me know what you think about the proposed way?
>>>>>>
>>>>>> [1] actually, the 1st argument in ASSERT/EXPECT_EQ is assumed to be an
>>>>>> expected value, so you should write ASSERT_EQ(5, a) not ASSERT_EQ(a, 5)
>>>>>> [2]
>>>>>> https://github.com/google/googletest/blob/master/googletest/docs/V1_7_FAQ.md#why-does-google-test-support-expect_eqnull-ptr-and-assert_eqnull-ptr-but-not-expect_nenull-ptr-and-assert_nenull-ptr
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> — Igor
>>>>>>
>>>>>>> On Sep 8, 2016, at 10:18 AM, David Holmes <david.holmes at oracle.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>>>> You should not have to cast NULL:
>>>>>>>>>
>>>>>>>>> 77   ASSERT_EQ(i, (Integer*) NULL) << "Not in the list";
>>>>>>>>>
>>>>>>>>> why is this needed? Is this something broken in gtest ??
>>>>>>>> There is no ASSERT_NULL (we likely should add it) in GTest so ASSERT_EQ
>>>>>>>> is used.
>>>>>>>> ASSERT_EQ is pretty strict about types so since NULL resolves to long
>>>>>>>> int (at least on my host) it cannot be compared with Integer*.
>>>>>>> I'm unhappy about this, it is a deficiency in Gtest. We should not be
>>>>>>> casting  0 or NULL to specific pointer types.
>>>>>>>
>>>>>>> cc'ing Kim to get his opinion on the best way to handle this.