[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

_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev