RFR(S): 8165433: Convert Test_linked_list to Gtest
david.holmes at oracle.com
Fri Sep 9 05:03:23 UTC 2016
On 8/09/2016 10:32 PM, Kirill Zhaldybin wrote:
> Thank you for clarifying this!
Indeed! Thank you very much.
> Here are a new WebRev:
> 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! :)
> Thank you.
> Regards, Kirill
> On 08.09.2016 12:22, Igor Ignatyev wrote:
>> 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.
>> 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,
>>  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?
>>  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)
>> — Igor
>>> On Sep 8, 2016, at 10:18 AM, David Holmes <david.holmes at oracle.com>
>>>>> 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.
More information about the hotspot-runtime-dev