RFR(S): 8165433: Convert Test_linked_list to Gtest

[Kirill Zhaldybin]

Igor,

Thank you for clarifying this!

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?

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.