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

[Michael Catanzaro]

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();
            }
        }

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