Closed Bug 1113229 Opened 5 years ago Closed 5 years ago

webrtc/sdp tests: fix -Wconversion-null warning and mark FAIL_ON_WARNINGS

Categories

(Core :: WebRTC: Signaling, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: cpeterson, Assigned: cpeterson)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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
Attachment #8538613 - Flags: review?(adam)
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);
Attachment #8538613 - Flags: review?(adam) → review+
(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
https://hg.mozilla.org/mozilla-central/rev/52326a6d6a65
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.