Android gcc is warning that ASSERT_EQ is comparing h264_parameters->level_asymmetry_allowed (uint16_t type) with a bool: > media/webrtc/signaling/test/sdp_unittests.cpp:1490:165 [-Wconversion-null] converting 'false' to pointer type for argument 1 of 'char testing::internal::IsNullLiteralHelper(testing::internal::Secret*)' Green Try build: https://tbpl.mozilla.org/?tree=Try&rev=f3cf90b6de7f

Comment on attachment 8538613 [details] [diff] [review] android_sdp_Wconversion-null.patch Review of attachment 8538613 [details] [diff] [review]: ----------------------------------------------------------------- r+, contingent on adjusting the macros per the comments below. ::: media/webrtc/signaling/test/sdp_unittests.cpp @@ +1486,5 @@ > static_cast<SdpFmtpAttributeList::H264Parameters*>( > video_format_params[0].parameters.get())); > ASSERT_EQ((uint32_t)0x42a01e, h264_parameters->profile_level_id); > ASSERT_EQ(0U, h264_parameters->packetization_mode); > + ASSERT_EQ(0U, h264_parameters->level_asymmetry_allowed); Since this really is being used as a boolean flag, I think this is clearer: ASSERT_FALSE(h264_parameters->level_asymmetry_allowed) @@ +1502,5 @@ > static_cast<SdpFmtpAttributeList::H264Parameters*>( > video_format_params[1].parameters.get()); > ASSERT_EQ((uint32_t)0x42a00d, h264_parameters->profile_level_id); > ASSERT_EQ(1U, h264_parameters->packetization_mode); > + ASSERT_EQ(1U, h264_parameters->level_asymmetry_allowed); And this is actually fragile, in that it's testing the value too strictly. There are ways the code being tested can be correct, but which would cause this test to fail. The test should instead treat it as a boolean: ASSERT_TRUE(h264_parameters->level_asymmetry_allowed);

(In reply to Adam Roach [:abr] from comment #1) > > + ASSERT_EQ(1U, h264_parameters->level_asymmetry_allowed); > > And this is actually fragile, in that it's testing the value too strictly. > There are ways the code being tested can be correct, but which would cause > this test to fail. The test should instead treat it as a boolean: > > ASSERT_TRUE(h264_parameters->level_asymmetry_allowed); I don't think that will compile. gtest's ASSERT_TRUE/FALSE macros expect a real bool type (gtest's AssertionResult class has an explicit bool ctor), but level_asymmetry_allowed is a uint16_t. Do you prefer something like this: > // convert uint16_t to bool using the "clown eyes" operator !! > ASSERT_TRUE(!!h264_parameters->level_asymmetry_allowed); or this? > ASSERT_NE(0U, h264_parameters->level_asymmetry_allowed);

(In reply to Chris Peterson (:cpeterson) from comment #2) > (In reply to Adam Roach [:abr] from comment #1) > > > + ASSERT_EQ(1U, h264_parameters->level_asymmetry_allowed); > > > > And this is actually fragile, in that it's testing the value too strictly. > > There are ways the code being tested can be correct, but which would cause > > this test to fail. The test should instead treat it as a boolean: > > > > ASSERT_TRUE(h264_parameters->level_asymmetry_allowed); > > I don't think that will compile. gtest's ASSERT_TRUE/FALSE macros expect a > real bool type (gtest's AssertionResult class has an explicit bool ctor), > but level_asymmetry_allowed is a uint16_t. Do you prefer something like this: > > > // convert uint16_t to bool using the "clown eyes" operator !! > > ASSERT_TRUE(!!h264_parameters->level_asymmetry_allowed); > > or this? > > > ASSERT_NE(0U, h264_parameters->level_asymmetry_allowed); I'm okay landing this: > ASSERT_TRUE(!!h264_parameters->level_asymmetry_allowed); But would find this preferable: > ASSERT_TRUE(static_cast<bool>(h264_parameters->level_asymmetry_allowed));

I landed the patch with the static_cast<bool> you preferred: https://hg.mozilla.org/integration/mozilla-inbound/rev/52326a6d6a65