Generalize ColorComponents to support an arbritrary number of components (though really we will only ever want 3, 4, or 5)
Created attachment 424571 [details] Patch
Comment on attachment 424571 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424571&action=review > Source/WebCore/platform/graphics/ColorComponents.h:66 > + template<size_t I> > constexpr T get() const; Classic example that I would find more readable on a single line instead of split across two. But maybe this is a losing battle since so many template lines get too long if you do that. > Source/WebCore/platform/graphics/ColorComponents.h:71 > + std::array<T, N> components; Funny that this really is "just an array" yet can you even construct it from an array? > Source/WebCore/platform/graphics/ColorMatrix.h:125 > + static_assert(ColorComponents<float, 4>::Size >= RowCount); Seems like we might need a typedef for ColorComponents<float, 4>. Honestly, ColorComponents<float, 4>::Size is a funny way to write "4"; with a typedef short name in this function it might read nicely. > Source/WebCore/platform/graphics/filters/FEMorphology.cpp:98 > + return PackedColor::RGBA { makeFromComponents<SRGBA<uint8_t>>(components) }; Do you have to write the type name here? Can’t we just use braces?
(In reply to Darin Adler from comment #2) > Comment on attachment 424571 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=424571&action=review > > > Source/WebCore/platform/graphics/ColorComponents.h:66 > > + template<size_t I> > > constexpr T get() const; > > Classic example that I would find more readable on a single line instead of > split across two. But maybe this is a losing battle since so many template > lines get too long if you do that. I keep wavering on this one. I went in pretty hard for it in ColorTypes.h, but sometimes it just looks weird. template<size_t I> constexpr T get() const; template<size_t Start, size_t End> constexpr ColorComponents<T, End - Start> subset() const; vs template<size_t I> constexpr T get() const; template<size_t Start, size_t End> constexpr ColorComponents<T, End - Start> subset() const; I think maybe it's the return types not lining up? There is also the problem of template functions in template classes, like: template<typename T, size_t N> template<size_t Start, size_t End> constexpr ColorComponents<T, End - Start> ColorComponents<T, N>::subset() const { ColorComponents<T, End - Start> result; for (std::remove_const_t<decltype(T::Size)> i = Start; i < End; ++i) result[i - Start] = components[i]; return result; } where splitting the two templates onto separate lines helps (in my view) to clarify the structure. > > > Source/WebCore/platform/graphics/ColorComponents.h:71 > > + std::array<T, N> components; > > Funny that this really is "just an array" yet can you even construct it from > an array? the point of this class is to avoid ever having color components in a raw std::array, so thankfully that hasn't come up yet. > > > Source/WebCore/platform/graphics/ColorMatrix.h:125 > > + static_assert(ColorComponents<float, 4>::Size >= RowCount); > > Seems like we might need a typedef for ColorComponents<float, 4>. Honestly, > ColorComponents<float, 4>::Size is a funny way to write "4"; with a typedef > short name in this function it might read nicely. Yeah, but in a follow up this function will be generalized to work with non-4 component ColorComponents, so that will go away. > > > Source/WebCore/platform/graphics/filters/FEMorphology.cpp:98 > > + return PackedColor::RGBA { makeFromComponents<SRGBA<uint8_t>>(components) }; > > Do you have to write the type name here? Can’t we just use braces? Yeah, it's required since the constructor is explicit, which I did on purpose to make sure people have to specify which type it is. It's a bit redundant in cases where you are returning, but overall I think being more explicit here is better.
Committed r275206: <https://commits.webkit.org/r275206> All reviewed patches have been landed. Closing bug and clearing flags on attachment 424571 [details].
<rdar://problem/76006720>