[webkit-dev] Idiom for functions with all return values in a switch case

[Maciej Stachowiak]

> On May 9, 2017, at 11:06 AM, Michael Catanzaro <mcatanzaro at 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 at lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev