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

[Maciej Stachowiak]

> On May 9, 2017, at 12:05 PM, Alfonso Guerra <huperniketes at gmail.com> wrote:
> 
> 
> 
> On May 9, 2017 2:07 PM, "Michael Catanzaro" <mcatanzaro at igalia.com <mailto: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;
>            }
>        }
>  <x-redundant-cluster-toggle://0>
> 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;
>            }
>        }
> 
> 
> Out of curiosity, why is using a switch statement better than defining an array to hold the return values?

Some possible reasons:

- The compiler can likely turn the switch statement into simple arithmetic (maybe even just a move if the enum values happen to match).
- A lookup table array would require an explicit range check to avoid out of bounds reads.
- The lookup table approach doesn't give you a compiler warning if you miss one of the enum values.

> 
> 
> 
> 
> Alfonso Guerra
> Founder/CEO
> Apokalypse Software Corp
> @Huperniketes
> (626) 667-4285
> 
> 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 <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=171851>
> https://bugs.webkit.org/show_bug.cgi?id=171866 <https://bugs.webkit.org/show_bug.cgi?id=171866>
> https://bugs.webkit.org/show_bug.cgi?id=171867 <https://bugs.webkit.org/show_bug.cgi?id=171867>
> 
> Michael
> 
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org <mailto:webkit-dev at lists.webkit.org>
> https://lists.webkit.org/mailman/listinfo/webkit-dev <https://lists.webkit.org/mailman/listinfo/webkit-dev>
> 
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org <mailto:webkit-dev at lists.webkit.org>
> https://lists.webkit.org/mailman/listinfo/webkit-dev <https://lists.webkit.org/mailman/listinfo/webkit-dev>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20170509/07c2fdc5/attachment.html>