Improve FourCC to use more inlining, fix incorrect mix of WEBCORE_EXPORT on entire class with inline functions
Created attachment 449712 [details] Patch
Created attachment 449741 [details] Patch
Created attachment 449758 [details] Patch
OK, after two false starts, this one is working and ready for review. For FourCC itself, this makes the code easier to read, more elegant, reduces reliance on reinterpret_cast and assumptions that all platforms are little-endian, and even makes it a bit more efficient, although mostly in places where efficiency isn’t critical. On the other hand, for the ISOBox classes, while this fixes the problem where the coding style checker says "never use inline functions in a class marked WEBCORE_EXPORT", it makes the code less elegant. The underlying cause is that because these seemingly-simple structures are instead a class hierarchy that descends from a polymorphic class with a virtual destructor. That pattern isn’t working all that well to keep the code simple, and at some point it might be worth exploring whether it’s really needed, and considering alternatives.
Comment on attachment 449758 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449758&action=review > Source/WebCore/platform/graphics/FourCC.h:34 > + constexpr FourCC(uint32_t value) : value { value } { } Would like to remove this so people don’t accidentally use it in cross-platform code instead of string literals, but sadly it’s too valuable for interoperating with Cocoa API that uses unsigned 32-bit ints for this, the type FourCharCode and such, so I was unable to delete it at this time. > Source/WebCore/platform/graphics/FourCC.h:48 > + value = data[0] << 24 | data[1] << 16 | data[2] << 8 | data[3]; Should use member construction syntax for this instead of assignment.
Comment on attachment 449758 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449758&action=review > Source/WebCore/platform/graphics/FourCC.cpp:44 > +std::array<char, 5> FourCC::string() const We could make this inline and put it in the header. Also could make it constexpr.
Comment on attachment 449758 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449758&action=review > Source/WebCore/platform/graphics/FourCC.cpp:36 > + static_cast<char>(string[0]), Are there any other rules worth asserting here? For instance, can a character be '\0'?
Comment on attachment 449758 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449758&action=review >> Source/WebCore/platform/graphics/FourCC.cpp:36 >> + static_cast<char>(string[0]), > > Are there any other rules worth asserting here? For instance, can a character be '\0'? In practice, I think these all need to be printables, in the range 0x20-0x7F. But not sure if it’s valuable in practice to check that, though. Making a FourCC successfully with values < 0x20 for some of the characters would not have any ill effect that I am aware of. Mainly the reason this function checks that the string is all ASCII is we don’t want to chop non-8-bit UTF-16 code units and use just the low 8 bits. I think I won’t add more checks. Related: The '\0' below is just there so we can reuse the constructor that is mainly present for ASCII literals, and it will be collapsed away by inlining.
Created attachment 449771 [details] Patch for landing
Committed r288426 (246317@trunk): <https://commits.webkit.org/246317@trunk>
<rdar://problem/87947650>