Bug 179706 - Add a compile-time-checked string literal initializer for FourCC.
Summary: Add a compile-time-checked string literal initializer for FourCC.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on: 179499
Blocks:
  Show dependency treegraph
 
Reported: 2017-11-14 16:34 PST by Jer Noble
Modified: 2017-11-16 09:17 PST (History)
7 users (show)

See Also:


Attachments
Patch (7.18 KB, patch)
2017-11-14 16:37 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (8.23 KB, patch)
2017-11-15 08:47 PST, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2017-11-14 16:34:14 PST
Add a compile-time-checked string literal initializer for FourCC.
Comment 1 Jer Noble 2017-11-14 16:37:16 PST
Created attachment 326941 [details]
Patch
Comment 2 Michael Catanzaro 2017-11-14 19:13:30 PST
Comment on attachment 326941 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326941&action=review

> Source/WebCore/platform/graphics/FourCC.h:36
> +    template<std::size_t N>
> +    constexpr FourCC(const char (&data)[N])

Oh wow, clever.

EWS is green, but I'll test it with GCC soon anyway. Thanks!
Comment 3 Michael Catanzaro 2017-11-14 20:12:15 PST
(In reply to Michael Catanzaro from comment #2)
> EWS is green, but I'll test it with GCC soon anyway. Thanks!

You missed ISOVTTCue.cpp.
Comment 4 Jer Noble 2017-11-15 08:47:38 PST
Created attachment 326986 [details]
Patch for landing
Comment 5 Jer Noble 2017-11-15 09:02:39 PST
Just for reference, I verified with godbolt.org that the following (made up) function:

bool isSettingsBox(uint32_t name)
{
    return FourCC(name) == vttSettingsBoxType();
}

Compiles down to to a single assembly integer equality test with -O3:

isSettingsBox(unsigned int):
  cmp edi, 1937011815
  sete al
  ret
Comment 6 WebKit Commit Bot 2017-11-15 11:06:56 PST
Comment on attachment 326986 [details]
Patch for landing

Clearing flags on attachment: 326986

Committed r224887: <https://trac.webkit.org/changeset/224887>
Comment 7 WebKit Commit Bot 2017-11-15 11:06:58 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2017-11-15 12:06:42 PST
<rdar://problem/35566955>
Comment 9 Michael Catanzaro 2017-11-15 13:23:19 PST
It broke the build with GCC 5. I think the best option is to add a fallback non-constexpr constructor just for GCC 5, with a note to remove it when we require GCC 6 (which is planned for May). I'm testing a patch now.
Comment 10 Don Olmstead 2017-11-15 18:13:03 PST
(In reply to Michael Catanzaro from comment #9)
> It broke the build with GCC 5. I think the best option is to add a fallback
> non-constexpr constructor just for GCC 5, with a note to remove it when we
> require GCC 6 (which is planned for May). I'm testing a patch now.

Also seems like WinCairo with VS2015 does not like this either. Complains that not all values are initialized. Bots seem fine. Not sure why AppleWin isn't having an issue.

Really need to move to VS2017 across the board
Comment 11 Don Olmstead 2017-11-15 18:16:58 PST
Seems like AppleWin hit this as well https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/6072/steps/compile-webkit/logs/stdio

C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\platform\graphics\FourCC.h(36): error C2476: 'constexpr' constructor does not initialize all members (compiling source file C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\platform\graphics\avfoundation\cf\InbandTextTrackPrivateAVCF.cpp) [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
  C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\platform\graphics\FourCC.h(48): note: 'WebCore::FourCC::value' was not initialized by the constructor (compiling source file C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\platform\graphics\avfoundation\cf\InbandTextTrackPrivateAVCF.cpp)
  C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\platform\graphics\iso\ISOVTTCue.h(45): note: see reference to function template instantiation 'WebCore::FourCC::FourCC<5>(const char (&)[5])' being compiled (compiling source file C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\platform\graphics\avfoundation\cf\InbandTextTrackPrivateAVCF.cpp)