> On May 9, 2017, at 11:06 AM, Michael Catanzaro <mcatanz...@igalia.com> wrote:
>
> Hi,
>
> Consider this function:
>
> static WKAutoplayEvent toWKAutoplayEvent(WebCore::AutoplayEvent event)
> {
> switch (event) {
> case
> WebCore::AutoplayEvent::DidEndMediaPlaybackWithoutUserInterference:
> return
> kWKAutoplayEventDidEndMediaPlaybackWithoutUserInterference;
> case WebCore::AutoplayEvent::DidPlayMediaPreventedFromPlaying:
> return kWKAutoplayEventDidPlayMediaPreventedFromAutoplaying;
> case WebCore::AutoplayEvent::DidPreventMediaFromPlaying:
> return kWKAutoplayEventDidPreventFromAutoplaying;
> case WebCore::AutoplayEvent::UserDidInterfereWithPlayback:
> return kWKAutoplayEventUserDidInterfereWithPlayback;
> case
> WebCore::AutoplayEvent::UserNeverPlayedMediaPreventedFromPlaying:
> return kWKAutoplayEventUserNeverPlayedMediaPreventedFromPlaying;
> }
> }
>
> It will trigger this GCC warning:
>
> [3490/4357] Building CXX object
> Source...bKit2.dir/UIProcess/API/C/WKPage.cpp.o
> ../../Source/WebKit2/UIProcess/API/C/WKPage.cpp: In static member function
> ‘static WKAutoplayEvent WKPageSetPageUIClient(WKPageRef, const
> WKPageUIClientBase*)::UIClient::toWKAutoplayEvent(WebCore::AutoplayEvent)’:
> ../../Source/WebKit2/UIProcess/API/C/WKPage.cpp:2277:9: warning: control
> reaches end of non-void function [-Wreturn-type]
> }
> ^
>
> Such functions are very common in WebKit. What should be our preferred idiom
> for suppressing this warning?
>
> https://bugs.webkit.org/show_bug.cgi?id=171851 suggests this approach:
>
> static WKAutoplayEvent toWKAutoplayEvent(WebCore::AutoplayEvent event)
> {
> switch (event) {
> // ...
> }
>
> ASSERT_NOT_REACHED();
> return 0;
> }
>
> That works for the cases in that bug, but it won't work in this case, because
> the return value here is an enum, so a cast would be needed.
>
> I've been putting a RELEASE_ASSERT_NOT_REACHED() in the default case:
>
> static WKAutoplayEvent toWKAutoplayEvent(WebCore::AutoplayEvent event)
> {
> switch (event) {
> // ...
> default:
> RELEASE_ASSERT_NOT_REACHED();
> }
> }
I think this second option may suppress the warning when you have forgotten to
list one of the enum values, since there is now a default case. I believe
that's the reason for the recommended option.
>
> Of course, that would work just as well coming after the switch. This is more
> general as it works for enums, but RELEASE_ASSERT_NOT_REACHED() is a bunch of
> typing. I've seen CRASH() used frequently as well, but what's the point of
> having RELEASE_ASSERT_NOT_REACHED() at all if we are using CRASH() directly?
>
> Opinions?
>
> Bugs to close once we've decided how to handle this:
>
> https://bugs.webkit.org/show_bug.cgi?id=171851
> https://bugs.webkit.org/show_bug.cgi?id=171866
> https://bugs.webkit.org/show_bug.cgi?id=171867
>
> Michael
>
> _______________________________________________
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev
_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
