> On May 9, 2017, at 12:05 PM, Alfonso Guerra <hupernike...@gmail.com> wrote: > > > > On May 9, 2017 2:07 PM, "Michael Catanzaro" <mcatanz...@igalia.com > <mailto: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; > } > } > <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@lists.webkit.org <mailto:webkit-dev@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@lists.webkit.org <mailto:webkit-dev@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@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
